Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blocky Chroniclers #796

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Blocky Chroniclers #796

wants to merge 4 commits into from

Conversation

SamsTheNerd
Copy link
Member

Allows chroniclers to read/write blocks.

Blocks can opt in in three ways.

  1. implement ADIotaProvider on a Block Entity class.
  2. implement IBlockyIotaProvider on a Block class.
  3. manually register with HexAPI#registerBlockyIotaHolder

Option 1 will likely work best in most cases (since data is generally stored in a block entity), but option 2 is good for static reading or for reading that only depends on the environment (like akashic record). Finally, option 3 is provided for cases where you can't implement an interface, such as for another mod's block or for optional hex interop.

Copy link
Member

@object-Object object-Object left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicks, some questions. Otherwise I think it overall looks good, assuming you've tested everything.

@@ -34,4 +34,24 @@ default Iota emptyIota() {
* @return whether it is possible to write to this IotaHolder
*/
boolean writeable();

// a helper for read-only holders
interface ADIotaHolderReadOnly extends ADIotaHolder{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should there also be a WriteOnly version?

import net.minecraft.world.phys.Vec3

// a silly stupid wrapper for throwing arbitrary badness mishaps (the ones that have a .of(thing, stub) constructor.
class MishapBad {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should just be a function in ActionUtils or something. Or if it has to be its own class for some reason, it should at least be abstract.

Comment on lines +96 to +110
@Override
public CompoundTag readIotaTag(){
if(pattern == null){
return IotaType.serialize(emptyIota());
}
return IotaType.serialize(new PatternIota(pattern));
}

@Override
public Iota readIota(ServerLevel world) {
if(pattern == null){
return emptyIota();
}
return new PatternIota(pattern);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels weird for the logic here to be duplicated... not sure how to fix it though.

Comment on lines +128 to +132
@Nullable
default ADIotaHolder findDataHolder(BlockPos pos, ServerLevel world){
return HexAPIImpl.getBlockyIotaHolder(world, pos);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels kinda weird to put something in IXplatAbstractions if it's not actually different per-platform. I see why it's there, I guess... maybe add a comment justifying it?

Comment on lines +980 to +985
iota: {
"": "место для хранения йоты",
read: "место, откуда можно читать йоты",
write: "место, куда можно записывать йоты",
readonly: "место, которое будет принимать %s",
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where's this from?

Comment on lines +995 to +1000
iota: {
"": "一个可以存储iota的地方",
read: "一个可以读出iota的地方",
write: "一个可以写入iota的地方",
readonly: "一个能够接受%s的地方",
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where's this from?

Comment on lines +69 to +79
if(!simulate){
if(iota instanceof PatternIota pIota){
this.pattern = pIota.getPattern();
sync();
}
if(iota instanceof NullIota || iota == null){
this.pattern = null;
sync();
}
}
return iota instanceof PatternIota || iota instanceof NullIota || iota == null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels weirdly duplicated, but idk how to fix it (other than moving to Kotlin).


val burstPos = target.map({ent -> if (ent is ItemEntity) {
// Special case these because the render is way above the entity
ent.position().add(0.0, 3.0 / 8.0, 0.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this logic could be moved to a utility function? eg ParticleSpray.getEntityBurstPos or something

@SamsTheNerd
Copy link
Member Author

I believe Petra was opposed to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

2 participants