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

Workaround Axe Enchantments on Paxel by Checking an Axe #7903

Merged

Conversation

ChampionAsh5357
Copy link
Contributor

Closes #7860

Changes proposed in this pull request:

Allows the paxel to have axe enchantments by checking whether the enchantment can be applied to an axe. I opted for this solution compared extending AxeItem since that class assumes the tag is MINEABLE_WITH_AXE while paxels have their own tags. This would mean that all of the logic associated with the tag would need to be replaced and swapped out. In favor of avoiding redundancy, it is simpler to just check whether the item is an axe item.

Modded enchantments should check whether the action can perform AXE_DIG, so it should remain compatible.

As this is a workaround, this should work until the mod loader API switches out the enchantment logic for checking tool actions instead.

@pupnewfster
Copy link
Member

The one issue with this solution is I believe sharpness is allowed on axes but only via enchanted books and not via the enchantment table. Or at least there was something like that when I looked into it some. Now I don’t know how much that actually matters but definitely needs some more thought. Cc: @thiakil

@thiakil
Copy link
Member

thiakil commented Oct 21, 2023

Can we use enchantment.category.canEnchant(AXE_item) instead? (preferably with the paxel's normal axe version)
I'm not a fan of making an itemstack here

Re sharpness: on a quick glance look like the methods an anvil uses will pass through here by default so should be fine

@pupnewfster
Copy link
Member

My point is it works for anvil here but it adds enchantments as possible to the enchantment table that are normally anvil only.

@thiakil
Copy link
Member

thiakil commented Oct 21, 2023

how so?

@ChampionAsh5357
Copy link
Contributor Author

Ah, I see what you're talking about. Enchantment#canEnchant is what's used to check whether the item can be enchanted with that instance at all while Enchantment#canApplyAtEnchantingTable checks only if the logic can be applied at an enchanting table. #canEnchant calls #canApplyAtEnchantingTable.

So, this would probably need to be addressed in NeoForge as a way to disambiguate the two. Otherwise, the only solution I could think of to maintain the logic is to mixin in a check somehow, which wouldn't be very mod-compatible since you revolve. Another solution would just be to say paxels are cool like that and can have the enchantments at the enchanting table.

This is assuming we are still using this method. The safest way would probably just be to extend AxeItem and just override all the tag checks with your own, which is a bit of a logic rewrite, but there's not much to do there.

I'll wait for further opinions before going forward here.

@thiakil
Copy link
Member

thiakil commented Oct 21, 2023

Which category are axe enchants? (or do they specifically check for axe?)

@ChampionAsh5357
Copy link
Contributor Author

There is none, it's essentially a hardcoded check in Enchantment#canEnchant within the DamageEnchantment. If anything, they fall under the DIGGER category by default.

@thiakil
Copy link
Member

thiakil commented Oct 21, 2023

Hmm I see. Ideally a forge fix for being hardcoded would be the way.

Though if you wanna see how it looks with paxels extending axe I'd take a look

@ChampionAsh5357
Copy link
Contributor Author

@thiakil here you are. It simply just extends AxeItem and changes any indirect checks on the tag to direct ones.

- Remove override of Forge added method
- Remove inlined tag
@pupnewfster
Copy link
Member

If we are doing it by making it extend the axe we may want to make:

  • canPerformAction call super instead of checking the default actions
  • Make usOn do something like:
InteractionResult axeResult = super.useOn(context);
if (result != InteractionResult.PASS) {
    return result;
}

And then remove the useAsAxe method. That way we won't have to worry about if mojang adds more features to the axe at some point as it will be handled by the super call.

Also in regards to overriding one vs both isCorrectToolForDrops I don't think we have a choice as in DiggerItem both are overridden to reference blocks rather than passthrough to the less specific variant. Though an alternate solution would be to add an AT in Mekanism Tools' AT file that definalizes and makes blocks protected and then update the tag key that is checked to being the paxel tag.

@ChampionAsh5357
Copy link
Contributor Author

Ok, I'll get around to it after the 5th since I won't have a computer that can run Java until then.

- Use suprt method to grab axe action tags in `#canPerformAction`
- Remove `#useAsAxe` in favor of the super `#useOn`
- Readd stack sensitive version of `#isCorrectToolForDrops` to deal with DiggerItem behaviors
@ChampionAsh5357
Copy link
Contributor Author

Okay, I've addressed the comments and added back in the second method for #isCorrectToolForDrops because of pup's comment. I could go for the definalization method if wanted, however.

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.

One more minor nitpick but other than that it looks pretty good (assuming when I test it things work as expected)

@thiakil thiakil merged commit e145b90 into mekanism:1.20.x Nov 14, 2023
2 checks passed
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.

Cannot use axe enchants on paxel
3 participants