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 ArrowLooseEvent not firing for bows #1189

Merged
merged 10 commits into from
Jul 4, 2024

Conversation

Spinoscythe
Copy link
Contributor

Fixes #1188

This was just a TODO left during porting to 1.20.5

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@Matyrobbrt Matyrobbrt added bug A bug or error 1.21 Targeted at Minecraft 1.21 labels Jun 25, 2024
@thomasglasser
Copy link
Contributor

Wouldn't ArrowShotEvent or ArrowFiredEvent be a better name for it 🤔 ArrowLoose sounds weird. Not the point of the PR ik but I thought I'd suggest a rename.

@thomasglasser
Copy link
Contributor

Actually it fires for bows no matter what the item is, which may not always be an arrow. BowShotEvent or BowFiredEvent would be better

@NewJumper
Copy link

or possibly rename it to ProjectileFireEvent to better match with the existing ProjectileImpactEvent?

@thomasglasser
Copy link
Contributor

thomasglasser commented Jun 30, 2024

See that doesn't work bc it doesn't fire for every projectile, it fires from BowItem and CrossbowItem

@NewJumper
Copy link

oh yea, that's true

Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

Patching the line with the instanceof ServerLevel is incorrect. While it prevents the arrow projectile from being spawned, it does not prevent the sound effect, the increment of the statistic, and the projectile item being consumed from the inventory.

(This should have been tested by adding an always-cancelling listener, which can be mimicked by modifying the EventHooks method to always return -1.)

The correct place to hook would be just after the getUseDuration call, in order to avoid those side-effects I mentioned above, and following the original patch as seen in 1.20.5.

@sciwhiz12 sciwhiz12 added the regression Worked previously but doesn't anymore label Jul 2, 2024
@sciwhiz12
Copy link
Member

Wouldn't ArrowShotEvent or ArrowFiredEvent be a better name for it 🤔

I would assume the name was decided to be the twin for ArrowNockEvent, which handles the bow being first drawn. (As to why they are not called BowDrawnEvent and BowFiredEvent, I don't exactly know.)

@HenryLoenwind
Copy link
Contributor

I would assume the name was decided to be the twin for ArrowNockEvent, which handles the bow being first drawn.

That event name is at least 11.5 years old; i.e. it dates back to the time when there was only a bow and a single type of arrow. So, while the Nock event is its twin, the name was chosen because it properly fit back then.

@Spinoscythe Spinoscythe requested a review from sciwhiz12 July 3, 2024 09:01
Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

Seems to work in my testing.

You need to clean up your patches. Re-add back unnecessarily removed blank lines, avoid renaming local variables without reason, etc.

patches/net/minecraft/world/item/BowItem.java.patch Outdated Show resolved Hide resolved
patches/net/minecraft/world/item/BowItem.java.patch Outdated Show resolved Hide resolved
@Spinoscythe Spinoscythe requested a review from sciwhiz12 July 3, 2024 14:24
@sciwhiz12 sciwhiz12 merged commit 498ea20 into neoforged:1.21.x Jul 4, 2024
6 checks passed
@Spinoscythe Spinoscythe deleted the arrlooseevent-not-firing branch July 4, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21 Targeted at Minecraft 1.21 bug A bug or error regression Worked previously but doesn't anymore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ArrowLooseEvent isn't fired when a bow is used
6 participants