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

[Quilt] Crash with Balm #278

Closed SplendidAlakey closed 1 year ago

SplendidAlakey commented 1 year ago

MC 1.19.2 QSL 4.0.0-beta12 Balm 4.5.2+0 Terrestria 5.0.1

When Terrestria and Balm are installed together on Quilt, the game crashes on startup. The issue doesn't happen on Fabric.

Let me know if this needs to be reported to Balm or Quilt instead.

Logs: https://gist.github.com/SplendidAlakey/dde2c2bdf95113aa01eb866725e0ee53

gniftygnome commented 1 year ago

I can reproduce this but I do not think the issue is in Terrestria. On a hunch I removed the included copies of FAPI libs from Terrestria but nothing changed.

  1. I can't find any way in which Terrestria mixes in or otherwise modifies the Minecraft method in question.
  2. Balm already has a bunch of reports of mixin failures including some quite similar to this one.
  3. Their mixin failures are apparently sporadic and likely depend on mod load order so adding or removing a mod could very easily trigger the problems without the added mod being directly involved in the failure.
  4. Terrestria does not officially support Quilt at the moment (but this is just a nail in the coffin).

I will leave this issue open for the moment but I don't intend to look into it further unless somebody is able to demonstrate how Terrestria is responsible for the Balm crash.

TelepathicGrunt commented 1 year ago

@gniftygnome It looks like Terrestria does a redirect to getAvailableMoisture which is what Balm is trying to modify variable at. https://github.com/TerraformersMC/Terraform/blob/1.19.3/terraform-dirt-api-v1/src/main/java/com/terraformersmc/terraform/dirt/mixin/MixinCropBlock.java

Redirects are not stackable and are an unsafe mixin. A push should be done to remove or replace all redirects with better alternatives that do not break other mod's mixins. You can even look into using MixinExtras if needed to wrap calls easier or try something similar to what balm is doing. https://github.com/LlamaLad7/MixinExtras/wiki/ModifyExpressionValue

Unless the crash can still be reproduced without a redirect, I would say the redirect here is the issue and cause. ShetiPhan also does a redirect to the same method spot and will conflict with Terrestria definitely so this is an issue.

gniftygnome commented 1 year ago

Ahh, thanks for tracking that down. So, this is basically https://github.com/TerraformersMC/Terraform/issues/61 as it turns out. I, too, prefer to avoid replacement mixins of all types, but old code has a way of being special. Terraform API needs to have all its mixins audited and modernized if applicable. Wouldn't it be nice if I could quit my day job and spend all day rewriting code for free... In any case, knowing what is going on here, I'll try and look into it when I've got some available time.

k2helix commented 1 year ago

Just wanted to inform that this continues to happen in latest versions of Balm, Terrestria and Quilt

gniftygnome commented 1 year ago

Terrestria as of our 1.19.4 betas no longer redirects. It is ... non-trivial to JiJ MixinExtras in a library mod ... so I hope that will get easier in the future. We remain incompatible with MoreTags because of their redirects, so we have added a breaks relationship for that mod.