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

Add support for hiding enchantments via hide flags #3284

Closed
wants to merge 2 commits into from
Closed

Add support for hiding enchantments via hide flags #3284

wants to merge 2 commits into from

Conversation

valaphee
Copy link
Contributor

@valaphee valaphee commented Sep 11, 2022

Description

This fixes #1852
by only sending an empty ench:[] tag for hidden enchantments, which will add the foil effect, which should not affect the items client behavior.

I didn't add hide flags for other items yet, because it wouldn't be possible to remove the additional data of fireworks etc. but other items, etc could be tested.

Testing

Has been tested on Waterfall connected to a 1.16 ViaVersion Paper server, with following command:
/minecraft:give @p acacia_button{Enchantments:[{id:aqua_affinity,lvl:1}],HideFlags:1} 1

@Camotoy
Copy link
Member

Camotoy commented Sep 11, 2022

I think this will break, at the very least, Curse of Binding.

@valaphee
Copy link
Contributor Author

I think this will break, at the very least, Curse of Binding.

Yep this would generate ghost items, but this scenario is very unlikely, as it also isn't a thing that can be done as survival playing player, or in normal circumstances.

But I would support empty ench tags, which also gives the item the foil effect in Java and Bedrock Edition, and the stored ench additionai hide flag.

@Camotoy
Copy link
Member

Camotoy commented Sep 11, 2022

I'd rather not intentionally introduce a bug if we can help it... Maybe we don't check the hidden flag if Curse of Binding is present?

@valaphee
Copy link
Contributor Author

valaphee commented Sep 12, 2022

I'd rather not intentionally introduce a bug if we can help it... Maybe we don't check the hidden flag if Curse of Binding is present?

Wouldn't do that, because then I would also have to exempt aqua affinity, frost walker, efficiency, etc. or it won't work.
or we only allow hiding unsafe enchantments, like enchantments on an acacia button.

For now I'll also revert hiding StoredEnchantments, because anvil also seems to be affected to not work anymore with hidden enchantments.

(In my commit I also changed the order of enchantments and storedenchantments, because this is how it will be processed in Java Edition, checked the source code for 1.19; and only enchantments, will always add an ench-Tag, even when empty)

… enchantment book additional details, add foil-effect for empty enchantments
@Camotoy
Copy link
Member

Camotoy commented Sep 29, 2022

Are the current changes actually beneficial to us in any way?

@valaphee
Copy link
Contributor Author

When there are 0 enchantments, it will still add the ench Tag, which would allow same behavior as Java Edition, and a way to create the foil effect without showing enchantments
(it will not be added when StoredEnchantments is used (also like Vanilla Java Edition))

@valaphee valaphee closed this Sep 30, 2022
@valaphee valaphee deleted the feature/hide-flags branch September 30, 2022 20:35
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.

Hide enchants attribute doesn't work
2 participants