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

Fix duplicate calls of certain click events #6309

Merged

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Jan 8, 2024

Description

Fixes a bug where right clicking on an entity can fire a right click event twice.

When you right click on an entity with no block in reach, a PlayerInteractEntityEvent is called like normal
However, a PlayerInteractEvent is also called, with the type of RIGHT_CLICK_AIR.
(This is also the case if there's a block in reach, but the SkriptEventHandler filters out non-air clicks with a useItemInHand value of DENY. air clicks ALWAYS have useItemInHand value of DENY for, like, non-usable items. So those aren't an issue). This causes the following:

on right click: # fires twice
on right click with <item>: # fires twice
on right click on player: # fires once
on right click on air: # fires once

The solution chosen is to combine the PIEE and PIE trackers, so we can only have one event of either type per tick from a player. This solves the issue, but is a breaking change (on right click on air no longer fires when clicking on an entity). Personally, I think this is acceptable, the behavior is technically a bug. The behavior now becomes the following:

on right click: # fires once
on right click with <item>: # fires once
on right click on player: # fires once
on right click on air: # doesn't fire

Target Minecraft Versions: any
Requirements: none
Related Issues: #6460

@sovdeeth sovdeeth added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.8 Targeting a 2.8.X version release labels Jan 8, 2024
@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Jan 8, 2024

#5250 Needs relation investigation.

Do you use ViaVersion?

@sovdeeth
Copy link
Member Author

sovdeeth commented Jan 8, 2024

#5250 Needs relation investigation.

Do you use ViaVersion?

Nah, this is completely separate from ViaVersion, though I bet it's the same issue. (i also don't use viaversion, to clarify).
The behavior sounds pretty much identical to what I'm seeing in my testing without viaversion.

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work Sovde ⚡

Is it possible to have tests for this? it seems like it can cause issues without a decent testing since we had this implementation for long time

@TheLimeGlass
Copy link
Collaborator

Good work Sovde ⚡

Is it possible to have tests for this? it seems like it can cause issues without a decent testing since we had this implementation for long time

Would need mocking usage, still possible, but over complicated.

@sovdeeth sovdeeth changed the base branch from dev/feature to dev/patch January 29, 2024 11:18
@sovdeeth sovdeeth added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Feb 29, 2024
@APickledWalrus APickledWalrus merged commit 51f0537 into SkriptLang:dev/patch Mar 1, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants