ldtteam / Structurize

Minecraft Structures
GNU General Public License v3.0
44 stars 45 forks source link

Chore: new Delegate blueprint iterator implementation #670

Closed MotionlessTrain closed 2 months ago

MotionlessTrain commented 3 months ago

Closes # Closes # Closes #

Changes proposed in this pull request:

Review please

MotionlessTrain commented 3 months ago

This is binary breaking, do we want that or not

Whoops, something I have not considered at all. If that is not okay, would renaming AbstractStructureIterator back to AbstractBlueprintIterator and moving the result enum back into that class solve it? Or would it be more involved?

Raycoms commented 3 months ago

This is binary breaking, do we want that or not

Whoops, something I have not considered at all. If that is not okay, would renaming AbstractStructureIterator back to AbstractBlueprintIterator and moving the result enum back into that class solve it? Or would it be more involved?

I'm not 100% sure. You can test it out by trying to load minecolonies with it and having a builder build sth

someaddons commented 3 months ago

btw small thing but I'd rename from delegate -> wrapper, since thats how other things are named aswell e.g. InventoryWrappers etc

MotionlessTrain commented 3 months ago

Yeah, for simplicity of things undoing the class split might be easier :D then you can do iface + delegate from iface which should be fine

I had the class split to make sure that the increment and decrement with skip condition code would be the same in both classes. I guess I just copy it over to AbstractDelegateBlueprintIterator with a todo that both functions could be extracted to a base class.

I first thought about using the delegate/wrapper there too, but that would mean I would need to duplicate the logic on minecolonies' side, where I would be implementing it. Extending the original base class to use for the delegate seemed to be impractical too

MotionlessTrain commented 3 months ago

Things found during binary compatibility test:

  1. NoClassDefFoundError, due to Result being moved to the interface
  2. Changed method signature:
    com.ldtteam.structurize.placement.AbstractBlueprintIterator com.ldtteam.structurize.placement.StructurePlacer.getIterator()

    This particular one is nasty, as I need to support returning delegate iterators too. I may need to extend AbstractBlueprintIterator by the delegate/wrapper (and ignore all of its fields) after all, to solve it.

Interestingly, those were the only two things I found. I simply changed StructurePlacer to store an AbstractBlueprintIterator (my version) instead of an IBlueprintIterator, and it somehow was enough for the builders, miner and quarrier to continue

To better be safe than sorry (no clue which other mods would be affected), I'm going to revert the split from AbstractBlueprintIterator into IBlueprintIterator, AbstractBlueprintIterator and AbstractStructureIterator, and inherit AbstractDelegateIterator from AbstractBlueprintIterator instead (and rename it to AbstractBlueprintIteratorWrapper, or something like that. Name is not set in stone yet). It will just ignore the base class' fields, and override the methods to use the wrapped iterator's methods instead

MotionlessTrain commented 2 months ago

This seems sufficient now for the use case in Minecolonies