N1nn1 / dye_depot

Enhances your Minecraft experience with 16 vibrant new dyes and various other tweaks
https://modrinth.com/mod/dye-depot
Other
11 stars 8 forks source link

Please add the breaks section to fabric.mod.json and list the mods yours breaks #19

Open Linguardium opened 7 months ago

Linguardium commented 7 months ago

https://github.com/N1nn1/dye_depot/blob/44ce788fe02e1590cb59cd1632468b3fe6e963d9/src/main/resources/fabric.mod.json#L31-L40

your mod forcibly adds things into an enum, which are traditionally immutable. this creates issues with other mods that explicitly use the enum for their mods. Since your mod is the one being invasive and breaking other mods, i suggest adding the breaks section and adding the mods that yours breaks.

You can remove stuff from it as you add compatibility patches for those mods, but as it stands, finding out which mod is breaking everyone is rather difficult

MehVahdJukaar commented 7 months ago

Question, where would this actually break mods? I use dyecolors alot in my mods and this mod seems to work with it. Only occurrence i see would be problematic are switch statements that cover all colors without having a default branch

Linguardium commented 7 months ago

mainly i see an issue with iterations with assumed values or array definitions for 16 and unchecked adding from the enum via ordinal. There is also the concern over byte packing due to original values fitting in 1 nibble while the new value exceeds that which may cause overflow issues.

MehVahdJukaar commented 7 months ago

Byte packing is an issue. I do use that and dye depot colours will rollback to vanilla ones for that. Not a major issue but still one. For hardcoded values I do think that it would be better practice to check enum length when accessing any enum instead or hardcoding the expected length

unilock commented 7 months ago

It would be better practice not to extend an enum via Mixin in the first place :p

That would be a bit like checking if (2 + 2 == 4): it's never not going to be true... unless someone changes the definition of 2, but it goes without saying that that wouldn't be a good idea.

MehVahdJukaar commented 7 months ago

honestly for this specific one ido think its the most compatible way to do it. Anythin that uses a dyecolor will automatically work with this mod without crazy ASM hacks and extensive integration needed. Provided one does not hardcode enum length or use switch statements or rely on unchecked byte packing for example. For my mods all the new colored block also get registered automatically meaning less compat work is needed as i just iterate over the enum to do that. there has been similar mods like this before and the most successful ones were the ones that extended the enum. like tinted

TheRealWormbo commented 6 months ago

The thing is, for working with multiple sets of blocks that are based on a dye color, it could be useful to have lookup functions that use the dye color. Also, I would assume that there are more situations where blocks use different textures based on the dye color rather than the dye colors themselves. The best case you could reasonably hope for is that blocks and items end up being rendered incorrectly, which I would already count as a potential break. How many mods that care about dye colors in some way are not broken out of the box when combined with Dye Depot?