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

[port/1.21] Fix issues relating to gameplay-enchantment support #1089

Merged
merged 10 commits into from
Jun 13, 2024

Conversation

Shadows-of-Fire
Copy link
Contributor

The switch to data enchantments has left GetEnchantmentLevelEvent and IItemExtension#getAllEnchantments inoperable due to the inability to resolve Holders, which are required to populate an ItemEnchantments instance. This PR provides the necessary RegistryLookup<Enchantment> to these contexts, retrieving it "as safely as possible".

The term "as safely as possible" means compromising between accessing magic global state and patching a ton of locations. The primary technique used to access the RegistryLookup is by retrieving it from the existing Holder.Reference objects, or whatever other context is available (such as an entity).

In two cases, there was no relevant context available, which means that the global state has to be accessed through the new CommonHooks.resolveLookup function. The cases were:

  1. ItemEnchantmentsPredicate.Enchantments#matches, which only has ItemStack context, and
  2. EnchantmentHelper#runIterationOnItem(ItemStack, EnchantmentHelper.EnchantmentVisitor), which does not have any access to a server or level.

All other cases use available local state to resolve the RegistryLookup.

In addition, this PR resolves a todo in ArrowItem#isInfinite, updating it to reflect current systems.

@Shadows-of-Fire Shadows-of-Fire added the 1.21 Targeted at Minecraft 1.21 label Jun 11, 2024
@Shadows-of-Fire Shadows-of-Fire self-assigned this Jun 11, 2024
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Jun 11, 2024

  • Publish PR to GitHub Packages

Last commit published: 7d5f052def8b9197a50e61bf46504329a7aa1156.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1089' // https://github.com/neoforged/NeoForge/pull/1089
        url 'https://prmaven.neoforged.net/NeoForge/pr1089'
        content {
            includeModule('net.neoforged', 'neoforge')
            includeModule('net.neoforged', 'testframework')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1089.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1089
cd NeoForge-pr1089
curl -L https://prmaven.neoforged.net/NeoForge/pr1089/net/neoforged/neoforge/21.0.0-alpha.1.21-rc1.20240613.033616/mdk-pr1089.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

@pupnewfster pupnewfster self-requested a review June 12, 2024 18:34
Copy link
Member

@pupnewfster pupnewfster left a comment

Choose a reason for hiding this comment

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

Left some comments, it also seems like IItemStackExtension#canApplyAtEnchantingTable(Enchantment) is currently a dead method? Is it still a possible hook? If not we should probably remove it. If it is a possible hook, then we likely want to hook it back up and change it to take a Holder<Enchantment> instead of an Enchantment

We also will need to make sure in the neo docs (cc: @ChampionAsh5357 ) that we specify the behavior of certain EnchantmentHelper methods compared to how we patch things. For example: EnchantmentHelper#hasAnyEnchantments just checks about tag based enchantments and not gameplay ones (which is correct from the point of view that it is only used by the grindstone in vanilla, but naming wise might confuse people).

One other thought is do we want to be patching EnchantmentHelper#hasTag to check gameplay enchantments? The uses I see are:

  • Beehive block, used to check if the stack is enchanted with an enchantment in EnchantmentTags.PREVENTS_BEE_SPAWNS_WHEN_MINING
  • Decorated pot, used to check if the stack is enchanted with an enchantment in EnchantmentTags.PREVENTS_DECORATED_POT_SHATTERING
  • Ice block, used to check if the stack is enchanted with an enchantment in EnchantmentTags.PREVENTS_ICE_MELTING
  • Infested blocks, used to check if the stack is enchanted with an enchantment in EnchantmentTags.PREVENTS_INFESTED_SPAWNS

I believe all of those are gameplay effects, so I think it would make sense for us to patch the helper method to be able to check our gameplay only ones.

patches/net/minecraft/core/Holder.java.patch Outdated Show resolved Hide resolved
+ * @param ammo The ammo stack (containing this item)
+ * @param bow The bow stack
+ * @param livingEntity The entity who is firing the bow
+ * @return True if the arrow is infinite
Copy link
Member

Choose a reason for hiding this comment

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

We should probably specify the behavior in the docs that unlike what we did in older mc versions, this doesn't return true by default for if the bow has the infinity enchant.

Though thinking about it, does not defaulting it like we used to mean that it is not possible to make an arrow not be infinite regardless of the enchantments on the bow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, yeah. Delegating that full control to the ArrowItem will require updating the method to basically just be a call wrapper for EnchantmentHelper.processAmmoUse

src/main/java/net/neoforged/neoforge/event/EventHooks.java Outdated Show resolved Hide resolved
This hook replaces `canApplyAtEnchantingTable` with a similar purpose, but adapted to the name changes made by vanilla.
@Shadows-of-Fire
Copy link
Contributor Author

IItemExtension#canApplyAtEnchantingTable was reimplemented as IItemExtension#isPrimaryItemFor(Holder<Enchantment>)

@Matyrobbrt Matyrobbrt added the bug A bug or error label Jun 13, 2024
@Matyrobbrt Matyrobbrt merged commit c9c4d98 into neoforged:port/1.21 Jun 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21 Targeted at Minecraft 1.21 bug A bug or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants