FoundationGames / Sandwichable

Customizable Sandwich mod for Minecraft
MIT License
32 stars 28 forks source link

[Breaking Incompatibility] Village generation has the potential to break when introduced with other mods that add village buildings #95

Closed emilyploszaj closed 2 years ago

emilyploszaj commented 3 years ago

Sandwichable's mixins that it uses to generate sandwich stands in villages are inherently incompatible with other modifications to village generation, depending on mixin load order. This is because they unconditionally replace the return injection with an identical one, for no reason at all. This will skip any later injections by other mods. If you're not aware, modifying reference variables in a mixin (any object) will be reflected outside of your mixin (unless you reassign, because objects are secretly just pointers), as it's a reference type (you don't get this behavior with primitives as they're value types). I have very recently implemented adding village structures to villages for one of my own mods, and I hope the code segment here provides some insight on how to compatibly generate your structures for use with other mods. It is notable as well that the code Sandwichable uses for this goal can be greatly simplified. Ship It requires only 2 files totaling 75 lines including imports to add 5 different structures, whereas Sandwichable has this spread out to a much larger set of code with a good bit of unnecessary allocation with wrapper types. This, of course, is not a necessary fix for the breaking incompatibility, though it should be mentioned for code maintainability at the least.

FoundationGames commented 3 years ago

I actually noticed this issue when porting the code over to Phonos. I did what I thought would fix the issue for Phonos, but forgot all about it for Sandwichable. Could you please test Phonos and make sure it doesn't break village generation in the same way?

emilyploszaj commented 3 years ago

Unfortunately it seems like phonos also is breaking, the fix is to simply not modify the return result, as it's unnecessary, you're structure was already added to the pool

FoundationGames commented 2 years ago

Fixed in 1.2-rc1