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.

Sign up for GitHub

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

Jump to bottom

Map new Mace enchantments for Bedrock clients #4653

Merged
merged 3 commits into from
May 10, 2024
Merged

Map new Mace enchantments for Bedrock clients #4653

merged 3 commits into from
May 10, 2024

Conversation

Teelair
Copy link
Contributor

@Teelair Teelair commented May 10, 2024

This PR fixes the new Mace enchantments, Breach, Density and Wind Burst not being mapped to Bedrock clients, causing users to be unable to see the contents of their inventory.

Prior to this PR, this would get spammed in console whenever a Bedrock user had one of these enchantments in their inventory:

[16:00:18 WARN]: [Geyser-Spigot] Could not translate packet ClientboundContainerSetContentPacket
java.lang.IllegalArgumentException: No enum constant org.geysermc.geyser.inventory.item.Enchantment.WIND_BURST
        at java.base/java.lang.Enum.valueOf(Enum.java:293) ~[?:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.inventory.item.Enchantment.valueOf(Enchantment.java:33) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.item.type.Item.remapEnchantment(Item.java:240) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.item.type.EnchantedBookItem.translateComponentsToBedrock(EnchantedBookItem.java:59) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.translator.item.ItemTranslator.translateToBedrock(ItemTranslator.java:148) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.translator.item.ItemTranslator.translateToBedrock(ItemTranslator.java:125) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.inventory.GeyserItemStack.getItemData(GeyserItemStack.java:156) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.translator.inventory.PlayerInventoryTranslator.updateInventory(PlayerInventoryTranslator.java:79) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.translator.protocol.java.inventory.JavaContainerSetContentTranslator.updateInventory(JavaContainerSetContentTranslator.java:86) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.translator.protocol.java.inventory.JavaContainerSetContentTranslator.translate(JavaContainerSetContentTranslator.java:70) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.translator.protocol.java.inventory.JavaContainerSetContentTranslator.translate(JavaContainerSetContentTranslator.java:40) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.registry.PacketTranslatorRegistry.translate0(PacketTranslatorRegistry.java:89) ~[Geyser-Spigot.jar:?]
        at Geyser-Spigot.jar/org.geysermc.geyser.registry.PacketTranslatorRegistry.lambda$translate$0(PacketTranslatorRegistry.java:69) ~[Geyser-Spigot.jar:?]
        at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174) ~[netty-common-4.1.97.Final.jar:4.1.97.Final]
        at io.netty.channel.DefaultEventLoop.run(DefaultEventLoop.java:54) ~[netty-transport-4.1.97.Final.jar:4.1.97.Final]
        at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[netty-common-4.1.97.Final.jar:4.1.97.Final]
        at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[netty-common-4.1.97.Final.jar:4.1.97.Final]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.97.Final.jar:4.1.97.Final]
        at java.base/java.lang.Thread.run(Thread.java:1583) ~[?:?]

onebeastchris
onebeastchris requested changes May 10, 2024
View reviewed changes
Copy link
Member

@onebeastchris onebeastchris 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 PR! Until Bedrock adds these enchantments itself, this seems like an okay temporary fix.

core/src/main/java/org/geysermc/geyser/item/type/Item.java Outdated Show resolved Hide resolved
onebeastchris
onebeastchris approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@onebeastchris onebeastchris 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 to me! thanks :)

Camotoy
Camotoy reviewed May 10, 2024
View reviewed changes
core/src/main/java/org/geysermc/geyser/item/type/Item.java Outdated
if (enchantment == Enchantment.JavaEnchantment.SWEEPING_EDGE) {
addSweeping(session, builder, level);
return null;
if (ENCHANTMENT_TRANSLATION_KEYS.containsKey(enchantment)) {
Copy link
Member

Choose a reason for hiding this comment

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

This can be replaced by moving the below get() above here then a null check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks

Camotoy
Camotoy approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@Camotoy Camotoy left a comment

Choose a reason for hiding this comment

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

Looks fine otherwise.

@onebeastchris onebeastchris merged commit 7801e35 into GeyserMC:master May 10, 2024
2 checks passed
@Teelair Teelair deleted the teelair/map-new-mace-encants branch May 10, 2024 23:21
XingLingQAQ pushed a commit to XingLingQAQ/Geyser that referenced this pull request Sep 25, 2024
@Teelair @XingLingQAQ
Map new Mace enchantments for Bedrock clients (GeyserMC#4653)
a93812d 
* Map new Mace enchantments for Bedrock clients

* Move to using a map for Java-only enchantments.

* Change to using null check for translationKey
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onebeastchris onebeastchris onebeastchris approved these changes

@Camotoy Camotoy Camotoy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Teelair @Camotoy @onebeastchris