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

Restrict modifying signs #2080

Merged
merged 3 commits into from Jun 28, 2023
Merged

Restrict modifying signs #2080

merged 3 commits into from Jun 28, 2023

Conversation

Jikoo
Copy link
Collaborator

@Jikoo Jikoo commented Jun 23, 2023

Restricts wax in general, so in the event that Mojang adds more waxables that Spigot doesn't fire a BlockPlaceEvent for, should be supported.

I'd like to take this opportunity to gripe about how inconsistent Spigot is about BlockPlaceEvents for actions that change states, even if cancelling interaction is much more performant. Book on a lectern? Yep, that's a block place, new state. Wax on a sign? Eh, the state changed but it's really just a sign still.

/e: Forgot the issue, whoops.
Closes #2060

@Jikoo Jikoo changed the title Restrict waxing signs Restrict modifying signs Jun 23, 2023
@Jikoo
Copy link
Collaborator Author

Jikoo commented Jun 23, 2023

Based on the fact that the SignEditEvent doesn't seem to be firing reliably from editing unwaxed signs it's safer to preemptively block sign interaction for editable signs. This does cause some redundant sign snapshotting for non-malicious usage, but that is much less likely to cause a performance impact than malicious usage.

@RoboMWM
Copy link

RoboMWM commented Jun 23, 2023

huh, why is SignEditEvent not always firing reliably?

@Jikoo
Copy link
Collaborator Author

Jikoo commented Jun 23, 2023

My best guess is that Mojang added it in a hacky way to handle the case where someone waxes a sign after you place it but before you finish writing the text. You'd think that that would basically just use the original placer as a flag, but I've seen a couple threads and issues about the subsequent unwaxed edits that suggest that it doesn't fire an event at all, meaning it's internally an entirely different code path.

@RoboMWM RoboMWM added this to the 16.18.2 milestone Jun 26, 2023
@RoboMWM RoboMWM merged commit 19cb0e3 into GriefPrevention:master Jun 28, 2023
1 check passed
@RoboMWM
Copy link

RoboMWM commented Jun 28, 2023

Idk if you can make a todo issue/improvement for when we switch to 1.20 API (probably will do that with v17 release, unless something major requires an API bump).

@Jikoo
Copy link
Collaborator Author

Jikoo commented Jun 28, 2023

Yeah, that's a good idea, I'll generate an issue from the line.

@Jikoo Jikoo deleted the dev/sign-wax branch June 28, 2023 20:23
@MacTh3Mac MacTh3Mac mentioned this pull request Aug 21, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Waxing signs without needing trust
2 participants