-
-
Notifications
You must be signed in to change notification settings - Fork 368
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 weird item mutation bug with EffHealth #6586
Fix weird item mutation bug with EffHealth #6586
Conversation
I would of put the new test into a regression however I didn't have a pr nor an issue for this and it was easier to just add it into the existing EffHealth.sk file |
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.
Just a quick review
} else { | ||
ItemUtils.setDamage(itemType, (int) Math2.fit(0, (ItemUtils.getDamage(itemType) + (isHealing ? -amount : amount)), itemType.getMaterial().getMaxDurability())); | ||
} |
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.
Due to 1.20.5 changes this is now technically bugable due to custom durability component. Forgot all about that.
In addition Damageable
ItemMeta is available on all items as well.
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.
Do you think this is worth delaying this addition, or should we get it in 2.6.5 and worry about the 1.20.5 changes later?
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.
Looking over it, is appears spigot has just thrown the the #setMaxDamage
method into Damageable
, I can quickly see how a test build of this interacts but as paper still has no stable release only betas I likely cannot push it for awhile longer.
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.
didn't wanna load up intellij for skript rn
src/test/skript/tests/regressions/pull-6586-effhealth item mutation.sk
Outdated
Show resolved
Hide resolved
src/test/skript/tests/regressions/pull-6586-effhealth item mutation.sk
Outdated
Show resolved
Hide resolved
Sometimes I'm dealing the items with the asynchronous way and the server crashing after this PR with a stack trace below.
I tried to revert this pr and the issue is gone. Also |
Doing anything async especially in bukkit is unsafe, in addition this really isn't the place to report an issue if a pr broke something. You can create an issue however based off the stacktrace I don't see much there that expresses it's caused by this change. The only skript related error is |
Description
This PR aims to fix a weird mutation bug that would happen on items when using
repair {_item1} and {_item2}
causing item2 to mutate into item1. This pr also adds itemtype support intoItemUtils#setDamage
andItemUtils#getDamage
since it was cleaner this way.The part that caused this bug in the first place was the
damageables.change()
usage. After removing it everything seems to still work as it did before.I spent far too long on this and ended up needing to clean up the class a bit along with remove some weird behavior skript was doing internally. If you have any questions feel free to ask. If you have any suggestions for more test please let me know as I'm not great at writing test with good explanations.
From testing currently I saw no breaking changes in parity and everything works as it was before except slots. While I was not 100% sure about them working prior I wanna say they most likely did however after some changes the parity broke forcing me into a slot check.
Here is a quick before and after to showcase the issue
Target Minecraft Versions: any
Requirements: none
Related Issues: none