Rework multiblock shape internals in preparation for future changes #1162
Conversation
Swedz
changed the title
…lso actually use it)
| import java.util.Set; | ||
| import aztech.modern_industrialization.machines.multiblocks.shape.member.MultiblockMember; | ||
| import aztech.modern_industrialization.machines.multiblocks.shape.member.SimpleMultiblockMember; | ||
| import java.util.*; |
| */ | ||
| public class ShapeTemplate { | ||
| public final Map<BlockPos, SimpleMember> simpleMembers = new HashMap<>(); | ||
| public final Map<BlockPos, HatchFlags> hatchFlags = new HashMap<>(); |
There was a problem hiding this comment.
I wouldn't make the hatch flags inherit from SimpleMultiblockMember, that's messy. We could group both a member and hatch flags into a record.
There was a problem hiding this comment.
The reason for grouping the hatch flags into HatchMultiblockMember is so the hatches can be serialized as part of the structure via. user input in a GUI much easier. I know this introduces a few instanceof checks here and there which isn't ideal. Other than that I'm not sure I agree that it's messy... what's your reasoning?
| private static void onSetup(FMLCommonSetupEvent event) { | ||
| for (ShapeTemplate template : templates) { | ||
| for (var member : template.members().values()) { | ||
| member.forceLoad(); |
There was a problem hiding this comment.
This is also quite messy, not a fan.
There was a problem hiding this comment.
Agreed that it is messy. Would you prefer members be loaded as-needed at runtime?
| protected final Map<BlockPos, MultiblockMember> members; | ||
|
|
||
| private boolean needsRematch = true; | ||
| private boolean matchSuccessful = false; |
There was a problem hiding this comment.
The matchedHatches field should really be removed IMO.
| import net.minecraft.world.level.block.state.BlockState; | ||
| import net.neoforged.neoforge.common.util.Lazy; | ||
|
|
||
| // TODO SWEDZ MULTIBLOCKS: remove methods that become irrelevant once structures have been implemented fully and we are |
There was a problem hiding this comment.
I'm not a big fan of leaving these TODOs around. I can live with it though.
There was a problem hiding this comment.
Sorry, I don't know of a better way to track this... 😅 I guess I could keep a list independently but it does help being able to see it in the code for me. I'll see what I can do to help myself there and remove these comments.
|
|
||
| // TODO SWEDZ MULTIBLOCKS: remove methods that become irrelevant once structures have been implemented fully and we are | ||
| // done making multiblocks using code | ||
| public abstract class MultiblockMember { |
There was a problem hiding this comment.
Should likely be an interface
|
|
||
| import net.minecraft.world.level.block.state.BlockState; | ||
|
|
||
| public abstract class MultiblockMemberTest { |
There was a problem hiding this comment.
Shouldn't these just be new types of multiblock members?
There was a problem hiding this comment.
The reason tests and members are separate is so we can have a hatch + simple member type (as well as some other types) that can have many different types of tests. I do think MultiblockMember should implement MultiblockMemberTest now that you point that out, though. Tests don't need to have a preview attached to them, whereas members do - because the member is the actual entry on the structure that will show in EMI/holograms/etc. If you see a better way to structure this, I am open to it.
| * allows us to load multiblock structure files on startup without any problems. | ||
| * </p> | ||
| */ | ||
| public record MultiblockMemberState(Lazy<BlockState> lazyState) { |
There was a problem hiding this comment.
Use Supplier (which is a supertype of Lazy).
2 participants
This is the first of a series of PRs that will overhaul how multiblocks are handled in MI. And is definitely one of the larger ones. I did some decently extensive testing using a world containing all MI multiblocks, but please do some testing of your own too.
Comments prefaced with
// TODO SWEDZ MULTIBLOCKS:are comments to remind myself in future PRs to update code at those places to help things not get missed in the process. These should all be gone by the end of this series of PRs.Here's a summary of the changes with some explanations. Some of the changes may seem odd at face value but they were made with future changes in mind. If there's anything I missed in this list and you find questionable, let me know.
ShapeTemplatehas been completely refactored to actually be immutable nowSimpleMemberhas been removed and replaced withMultiblockMemberSimpleMultiblockMemberandHatchMultiblockMember) since that is the minimal amount needed for MI at the moment (more will come with a future PR)MultiblockMemberTestwhich is separate so that user input can be parsed into multiple tests for a single member delimited by a semicolonMultiblockMemberStatehas some comments to explain why it is the way it is - but to further elaborate:@Nullable CompoundTag nbtfield so NBT of BlockEntities can be compared too. That is why this is a separate class instead of just referencingLazy<BlockState>or justBlockStateeverywhere. At the moment there's no real reason to useLazy<BlockState>- it is so that in the future when we are loading snbt files for the multiblock structures, we can load those before all mods blocks are registered and thus haveShapeTemplateinstances to pass to our machines on startup. Otherwise a lot more code would have to get updated to allow forShapeTemplatesbeing applied after startup which would get rather messy.BlockStates being lazy loaded where that hasn't been fully parsed during play. This could cause inconsistent results that I'm not really too interested in finding out what breaks because it's not immediately obvious. So I forcefully load all of the lazyBlockStates on startup in aFMLCommonSetupEvent(see the event inShapeTemplate- if you're not a fan of the use of an annotation event here I can move it toMI's constructor). If you think there is a better solution to this let me know, this is the best I came up with.ShapeTemplateare kept pretty much the same so that existing scripts in KJS shouldn't need any changing. However addons will need to adjust to account for the package and type changes.I request that #1159 be looked at before this PR so I can rebase here rather than there. Also, that PR is much smaller than this one :)