TerraformersMC / Terrestria

A Fabric mod enhancing the detail of Minecraft with unique and vibrant biomes. Inspired by ExtrabiomesXL.
https://www.curseforge.com/minecraft/mc-mods/terrestria
GNU Lesser General Public License v3.0
199 stars 43 forks source link

Crash with Immersive Weathering due to conflicting @Redirect mixins #314

Closed MehVahdJukaar closed 8 months ago

MehVahdJukaar commented 10 months ago

Redirect is unsafe and should almost never be used. In your case here https://github.com/TerraformersMC/Terrestria/blob/376ef56251c4e6866c932c8ee1f78815a44b1fd7/client/src/main/java/com/terraformersmc/terrestria/mixin/MixinBlockDustParticle.java#L17

Known to crash with Immersive Weathering (to which im reporting the same issue) which has a mixin pretty much identical to what you have. As far as i know block break particles just use whatever tint color is previded at tint index 0 so to not use it simply dont give any color to tint index 0 and use 1 for the block coloring

haykam821 commented 10 months ago

This mixin will be replaced with the event added in FabricMC/fabric#3146 in a future release.

MehVahdJukaar commented 10 months ago

hm why not just not use tint index 0 so we can get rid of the issue now? is there something I'm missing, it seems to me like the game already easily supports this kind of behavior without the need for that event or that hardcoded check for the matter

gniftygnome commented 10 months ago

Wontfix for 1.19.x, I think. For 1.20, it could be in the next release although if FAPI 0.85.0+1.20.1 breaks on 1.20 then we would need to drop support for 1.20 (I'm guessing this would be pretty much OK).

MehVahdJukaar commented 10 months ago

that didnt answer my question tho :/

gniftygnome commented 10 months ago

that didnt answer my question tho :/

The question is moot, since the FAPI feature in question is already released.

gniftygnome commented 10 months ago

I see this should be resolved in the next release of Immersive Weathering for 1.19.2, so my plan is still going to be leaving it as is in Terrestria for 1.19.2. We will release an update for 1.20.x and maybe backport it to 1.19.4 when we have time.

gniftygnome commented 10 months ago

Resolved in 5b4ffc266d3f5610be86d466e5116c52a10fcb58 but I will leave this issue open until 1.19.2 is also resolved by one of the other parties involved or I have time and decent Internet connectivity to commit a work-around to Terrestria for 1.19.

Zh0ny commented 9 months ago

@gniftygnome Did you have some idea how to port to 1.19.2 ? (I think you did it in commit 9821d52) The fix that you make is based fabric API >= 0.85.0, which have ParticleRenderEvents, 1.19.2 fabric API stops in 0.76.1 and don't have this class. Terrestria v5.0.10 says that: "Improve Create mod compatibility.", but I'm staying have errors. This is the first time I've tried to resolve an error in a Minecraft mod, so I'm a little lost, and I'm not familiar with minecraft modding environment.

gniftygnome commented 9 months ago

Immersive Weathering already has a commit in their repository to resolve the conflict, and 1.19.2 remains their primary version. In comparison, we generally aren't making new releases for old versions like 1.19.2. I would prefer to wait until they make a new release to resolve this issue.

Yes, I did make a release relatively recently to improve compatibility with Create in 1.19.2, but the issue was entirely on our end and the fix was trivial to backport. Neither is the case here.

gniftygnome commented 8 months ago

I don't know what the IW folks are doing, so as promised, I've released an update (v5.0.12) for 1.19 - 1.19.2 that should work around the crash with IW, by simply not loading our mixin if IW is loaded.