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: disable sign-coloring when edit-sign flag is false #4252

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

Tamikaschu
Copy link
Contributor

@Tamikaschu Tamikaschu commented Dec 1, 2023

Overview

Fixes #4247

Description

#4236 Adds sign coloring to edit-sign flag separately from the use flag. The use flag is typically used to allow interactions with shops etc, but players should not be able to just color every sign if the use flag is needed.

@Tamikaschu Tamikaschu requested a review from a team as a code owner December 1, 2023 16:30
@github-actions github-actions bot added the Bugfix This PR fixes a bug label Dec 1, 2023
@PierreSchwang
Copy link
Member

Generally a good addition, but maybe this code may utilize ItemTags, kind of like:

final org.bukkit.Tag<Material> dyesTag = Bukkit.getTag(org.bukkit.Tag.REGISTRY_ITEMS, NamespacedKey.minecraft("dyes"), Material.class);
if (dyesTag.isTagged(itemStack.getMaterial())) {
    // Interact with dye
}

I haven't been able to test that, nor do I know or found any information on when the dyes tag was introduced. If that works in all supported versions, it may be a more future proof solution and does not potentially break with new dyes added.

@Tamikaschu
Copy link
Contributor Author

I will test it out but we have to keep in mind that not only dyes but also Honeycombs and Glow Ink Sack can be applied on signs. Will they also have the dye tag?

@PierreSchwang
Copy link
Member

Yeah, forget what I said. Seems like my online source listed that as a item tag, but scavenging through the paper / minecraft 1.20.2 source does not contain such tag nor is there any other kind of tag to be respected in those Items or general sign manipulation.

@dordsor21
Copy link
Member

Is there potentially a better event to use here rather than cancelling the interact? The block changes state so presumably that'll fire an event

@Tamikaschu
Copy link
Contributor Author

Tamikaschu commented Dec 3, 2023

Did some research and asked around on PaperMC discord. It seems that there is no good alternative event to detect players applying dye to signs or waxing them apart from PlayerInteractEvent. Sign coloring was added 1.16 and waxing 1.19. Should that maybe be checked in two separat events for the versions?

@dordsor21
Copy link
Member

Did some research and asked around on PaperMC discord. It seems that there is no good alternative event to detect players applying dye to signs or waxing them apart from PlayerInteractEvent. Sign coloring was added 1.16 and waxing 1.19. Should that maybe be checked in two separat events for the versions?

Were any of the items added posted 1.16? If so it will need to done using strings as older servers will not have the Materials and so it will cause issues. No need for separate events though

@Tamikaschu
Copy link
Contributor Author

Tamikaschu commented Dec 3, 2023

Were any of the items added posted 1.16? If so it will need to done using strings as older servers will not have the Materials and so it will cause issues. No need for separate events though

Glow Ink Sac was added in 1.17. But if we would cancel PlayerInteractEvent when a player applies honey to a sign in versions below 1.20 it would cancel the Interact even though waxing is not even possible due to the version. Separating events would also allow some fine adjustment: we don't have to cancel interact when a sign is already waxed.

@dordsor21
Copy link
Member

In that case just have a version check when building the list of items and only add depending of version. No need to do that for "new" items though as it's all strings

Copy link
Member

@SirYwell SirYwell left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@NotMyFault NotMyFault merged commit 89511f0 into IntellectualSites:main Dec 6, 2023
9 checks passed
@Tamikaschu Tamikaschu deleted the fix/sign-coloring branch December 8, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix This PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coloring signs
6 participants