Skip to content

Fix disable-tripwire-updates option not cancelling tripwire hook updates#12129

Merged
lynxplay merged 4 commits into
PaperMC:mainfrom
Dqu1J:fix-tripwire-updates
Feb 17, 2025
Merged

Fix disable-tripwire-updates option not cancelling tripwire hook updates#12129
lynxplay merged 4 commits into
PaperMC:mainfrom
Dqu1J:fix-tripwire-updates

Conversation

@Dqu1J
Copy link
Copy Markdown
Contributor

@Dqu1J Dqu1J commented Feb 16, 2025

Fixes #12127

Currently the disable-tripwire-updates has a bug, it does NOT cancel updates from tripwire hook blocks, which change the attached state property of the tripwire.

This PR fixes this by blocking the calculateState method inside TripWireHookBlock from running.

This is justified as the methods only functionality is:

  • Checking nearby blocks to find a correct tripwire line
  • Setting attached appropriately
  • Also managing redstone signals and firing events related to them (which already never happens when disable-tripwire-updates is on as the tripwires never get powered)

TLDR it only does block updates and nothing else, half of which are already not being ran with disable-tripwire-updates, hence I think the best solution is to just cancel the entire method.

@Dqu1J Dqu1J requested a review from a team as a code owner February 16, 2025 16:38
@lynxplay lynxplay added the type: bug Something doesn't work as it was intended to. label Feb 16, 2025
Copy link
Copy Markdown
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Welcome to paper 🎉

Thank you for the PR!
I am a bit unsure on cancelling the entire method here.
We are only preventing updates to tripwires, not to the hooks.
Additionally, if something else ever happens in this method in the future, this change would make it very difficult for us to spot that during a version update.

Does moving this check down to the actual update of the tripwire also solve the issue correctly? (I'd imagine so)

@Dqu1J
Copy link
Copy Markdown
Contributor Author

Dqu1J commented Feb 17, 2025

Thanks for the review @lynxplay

I moved the check down to the part of code that updates the block, and ensured it only runs for tripwire and not tripwire hooks.

It still does function correctly like that and the tripwire doesn't get updated (all fields are false).

Though the hook now sets attached to true if it found a valid tripwire line when placed and doesn't update afterwards unless the other hook is broken. This might seem a little inconsistent, tell me if I should cancel updating specifically the attached property for the hook too (by removing the first condition inside the if statement). Just mentioning it, I don't know if it's a concern or not. Other than that all is good. 👍

@Dqu1J Dqu1J requested a review from lynxplay February 17, 2025 18:26
Copy link
Copy Markdown
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Thanks for the quick response!
I think that is fine, the tripwire hooks are not part of the cancelled update, even if the resulting state looks fine.
Complete nitpick on my end, but can you just move the if statement directly infront of the setBlock line.
E.g.

if (!config.disableUpdatesTripWire) level.setBlock(blockPos1,....) // Paper - rest of comment

That way, in future updates, the patch fails to apply if the setBlock line there changes and we can correct it.
Beyond that, LGTM

Dqu1J added a commit to Dqu1J/Paper that referenced this pull request Feb 17, 2025
@Dqu1J
Copy link
Copy Markdown
Contributor Author

Dqu1J commented Feb 17, 2025

Done that @lynxplay

Thank you again for the feedback & swift responses ❤️

@lynxplay lynxplay force-pushed the fix-tripwire-updates branch from 139af2a to 27f3a6f Compare February 17, 2025 18:56
@lynxplay lynxplay merged commit fd69140 into PaperMC:main Feb 17, 2025
@Dqu1J
Copy link
Copy Markdown
Contributor Author

Dqu1J commented Feb 17, 2025

yay 🎉🎉🎉🎉🎉
thank you thank you

@Dqu1J Dqu1J deleted the fix-tripwire-updates branch February 17, 2025 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug Something doesn't work as it was intended to.

Projects

Status: Merged

Development

Successfully merging this pull request may close these issues.

disable-tripwire-updates option is not cancelling tripwire hook updates

2 participants