-
Notifications
You must be signed in to change notification settings - Fork 275
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
fix incompatibility with lithium explosion exposure optimization #1937
base: master
Are you sure you want to change the base?
Conversation
You can test events by loading the built-in I'll try to take a look and merge later. |
Any chance this could get merged? The current latest versions of Lithium and Carpet are now crashing when used together. ( |
Can this be merged ASAP? |
This really needs to be merged on Carpet side. Both mods are essential and crucial for many players, and further optimization of explosions is always a good thing to see. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ModifyArg
would be less brittle than local capture
Apply code review by supersaiyansubtlety Co-authored-by: Will <[email protected]>
Right so due to the fact that this will prob never get merged as gnembon just doesnt seem to have the time ig to fully maintain carpet mod [kinda sad but i get it ig...] here is a complied jar for anyone who needs it, for 1.21, 1.21.1 |
Thank you @Ka1nnn 😄 |
You can use the compiled artifact for the PR instead of relying on builds from unknown sources |
Event script reports events normally, (with and without the optimizeTNT carpet rule active). I ran a set of farms from scicraft Blitz world and they all worked as intended (with and without optimizeTNT active), blast chambers, world eater etc.. |
Sorry, was busy.. explosion code has changed drastically. Is that still an issue? |
I worked around this on lithium's side |
But wouldn't it be better if carpet implements the fix so you don't need to do a workaround? |
When is this pr going to be merged, my don't want to have to modify and compile the pr branch myself anymore : ( |
For me, the same crash during server launch also happens when using Moonrise v0.1.0-beta.10, Minecraft v1.21.1 and Carpet v1.21.1-4. Someone has posted this on the Moonrise repo already: Tuinity/Moonrise#24 |
Lithium redirects the same call to replace the computation with a cached value lookup.
I don't see a way to avoid lithium's redirect, without getting rid of the optimization, which is why I am suggesting to change it in carpet.
This PR uses the
@Local
instead of CAPTURE_FAILHARD to get the local variable, because it is easier.I don't know how to use scarpet, so I couldn't test it, but it at least didn't crash the game when I exploded a tnt in a flat world