Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] REID compat broken with biome remote changes (Includes proposed fix) #18

Closed
2 tasks done
jchung01 opened this issue Jul 1, 2024 · 3 comments
Closed
2 tasks done

Comments

@jchung01
Copy link

jchung01 commented Jul 1, 2024

Version of Advanced Rocketry

1.12.2-2.0.11-1

Have you verified this is an issue in the latest unstable build

  • Y

Version of LibVulpes

1.12.2-0.4.2-25

Version of Minecraft

1.12.2

Does this occur without other mods installed

  • N

This is a compat issue with RoughlyEnoughIDs, tested with REID 2.1.1.

Crash report or log or visualVM (if applicable)

N/A; I can provide it, but it is not important as it is a mixin error and I know the root cause (see below).

Reproduction steps

  1. Assemble a rocket with a satellite configured for a biome changer remote.
  2. Launch the rocket.
  3. Trigger classloading of the BiomeHandler class. This can be done by using the remote or putting it in the satellite terminal. This will crash due to a mixin inject failure from REID.

Description of the problem

Hey there, I'm the pseudo-maintainer of RoughlyEnoughIDs at the moment and I came across an issue with your biome remote changes. I would like to coordinate a fix for it, which would make most sense on your side. I use a mixin to target the BiomeHandler class here in order to set the biome according to REID's biome format. You recently changed the signature of the method I'm targeting (the target I specify vs. the target in your changed code), so my mixin will fail to find its target and hard crash. I can add a new injector to target your method, but looking at what you changed, I'd prefer if you fixed it on your side.

The fix would be to (1) keep the old signature/logic of BiomeHandler#changeBiome(World, Biome, BlockPos) and (2) extract the regen_vegetation logic of the method to its own method. I don't think you're modifying the biome array when regenerating the vegetation, so REID would not need to touch this new method, just the old one as it usually does on the official Advanced Rocketry versions. Let me know if there's issues with this approach to fixing this compatibility issue.

@dercodeKoenig
Copy link
Owner

dercodeKoenig commented Jul 3, 2024

I would prefer if you update your code.
I checked my code and I dont see an easy solution to move the new code out without double calling the decoration method.
This is because when the biome remote is used it should regenerate the vegetation always but else it should only regenerate when the biome is changed.
If I move the code into the biome changer satellite it would always double call the decoration because it does not know if the biomehandler has already called decoration. If I use a boolean type for the biomechanger it would mess up your mixin too. I am sorry about that :(

@jchung01
Copy link
Author

jchung01 commented Jul 4, 2024

No worries, that's on me for thinking the fix would be straightforward on your side. I'll go ahead and fix it on my side by accounting for your method signature in my mixin then.

@jchung01
Copy link
Author

jchung01 commented Jul 4, 2024

Tested and should be resolved by TerraFirmaCraft-The-Final-Frontier/RoughlyEnoughIDs#60.

@jchung01 jchung01 closed this as completed Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants