fix: Match vanilla behavior on item use and release - #609
Conversation
|
|
||
| if self.active_item_use_hand() != Some(hand) { | ||
| self.inventory.lock().set_item_in_hand(hand, item); | ||
| return; |
There was a problem hiding this comment.
what happens here if on_use_tick damages the item (like a brush) and stops item use? returning early here drops those changes without writing them back to the inventory.
| let mut inventory = self.inventory.lock(); | ||
| let current_hand = inventory.get_item_in_hand_mut(hand); | ||
| if current_hand.item() == active.item() { | ||
| *current_hand = item.clone(); |
There was a problem hiding this comment.
this writes item back as long as the item id matches, which overwrites any changes made to the held stack during on_use_tick.
and because it writes item back here, hand_item down on line 455 is always going to equal item. so if item == hand_item will basically always be true and never hit the release_using_item branch.
vanilla checks if the hand still matches the stack from before onUseTick. if it changed during the tick, it releases instead of finishing and doesnt overwrite the hand.
| let result = behavior.finish_using(&mut item, &world, self); | ||
| let mut inventory = self.inventory.lock(); | ||
| let current_hand = inventory.get_item_in_hand_mut(hand); | ||
| if current_hand.item() == active.item() { |
There was a problem hiding this comment.
in vanilla completeUsingItem, it also checks !this.useItem.isEmpty() before calling finishUsingItem.
also why check current_hand.item() == active.item() after finish_using? finish_using is supposed to change the item type for things like soup or honey bottles turning into empty bowls/bottles.
There was a problem hiding this comment.
Isn't it possible for finish_using itself to modify the item? Without the check it would open up the same bug on_use_tick had, overwriting what finish_using did with the returned result.
I believe in vanilla it happens with a stack with more than 1 item of honey bottles, for example. With 2 honey bottles it would return the empty glass and modify the inventory to decrease the amount of items.
At least, I believe this can happen but there is a high chance I am wrong.
|
This pull request has conflicts with the base branch "master". Please resolve those so we can test out your changes. |
079f9f6 to
0051b9d
Compare
|
Conflicts have been resolved! 🎉 |
|
I believe I've fixed everything according to the reviews. Some of the tests use stand-in items because the real behavior for food/drinks still hadn't been merged. Now that #411 is merged, I'll later send a follow up commit replacing them with tests using real items. |
| // TODO: Vanilla checks !useOnRelease here. | ||
| // implemented in PR #469 |
There was a problem hiding this comment.
Didn't do only quick todo check, so no review
means this should be merged after 469?
There was a problem hiding this comment.
To keep it completely in line with vanilla behavior, yes.
There was a problem hiding this comment.
Also, do you mean that I shouldn't leave TODO comments?
3 participants
Type of change
Description
Fixes #529 by better mimicking vanilla behaviour on functions
tick_active_item_useandrelease_using_item.Previously the held item would be replaced with an empty stack and the
on_use_tickwould be run on the local item stack. Now, the held item remains unchanged andon_use_tickis run on a clone of the held item. After that it is checked if the item still matches the item that was being held, if it does overwrite to apply item changes, if it doesn't it returns early. This check is also done afterfinish_using_item.Also changed
release_using_itemto use the same pattern so that it matches vanilla since it had the same bug.How this was tested
Ran cargo fmt, cargo clippy, cargo check and typos. There aren't any tests that cover this bug and I also didn't make one since the necessary behaviour to replicate this is in PR #411, which isn't merged.
Screenshots / logs
Checklist
Classes / commands modified:
Additional notes
Although the bug was present in
release_using_itemI don't think any vanilla item would be able to trigger it, since the only item that changes to something different than an empty hand on release would be an elytra (which isn't used on hand). However, plugins/mods could trigger that bug.