quilt-standard-libraries icon indicating copy to clipboard operation
quilt-standard-libraries copied to clipboard

Enchantment API module

Open Vaerian opened this issue 2 years ago • 7 comments

Preface: I recognize this is not where the repo's priority is right now, and considering how much of a rough draft this PR is, don't waste your time reviewing it unless you care deeply about the direction of this specific module (i.e. you provided/wanted to provide input on issue #13).

This PR adds a custom enchantment API similar to the one discussed in #13. I'm making this a draft PR so the discussion around this API can have something to conceptualize what would be required to make these changes a reality. The specific structure of the implementation is still up for debate, but this is generally what it could look like.

Progress

  • [X] Support for customizable item groups for custom enchanted books
  • [X] Powerful "enchantment targets"
    • Developers have access to the World, PlayerEntity, enchantment power, enchantment level, amount of bookcases, ItemStack, BlockPos, BlockState, and BlockEntity.
  • [X] Enchantment weight can be defined per level/context
  • [X] Enchantment weight free from the rarity enum
  • [ ] Add proper handling of treasure logic to the enchantment context
  • [ ] Support for more a powerful anvil application context
  • [ ] Armor and tool tags as extensible replacements for the built-in enchantment targets (Should probably be a separate PR)
  • [ ] Event callbacks for random enchantment and anvil application
  • [ ] Enchantment compatibility takes into account enchantment level (potentially entire enchantment context)

As previously mentioned this is still definitely a draft. It is likely that the majority of the structure of this PR will change including everything from javadocs and class names, to implementation details and method signatures. Don't waste your time nit-picking every individual detail at this point (I'm sure something somewhere is misspelled). Focus more on the big picture.

Discussion Questions

  • What additional information could developers benefit from having in the enchantment context?
  • Should the equipment slot array system be modified? If so, how? Is there a way to allow expandability of EquipmentSlots for something like Trinkets?
  • Which library does the API belong in? Should it stay in content_other?
  • How should event callbacks for random enchantments and anvil application work?
  • What kind of information should developers have access to when determining the compatibility of enchantments?
  • Enchantments use a variety of values such as the amount of bookcases, the "power" of the enchantment table, the level of the enchantment (i.e. I, II, III, etc.), the enchantment tier (the three boxes in the menu), etc. This gets kind of confusing in code. Is there a way we can standardize these terms both in the API and in the mappings? If so, what should that look like?
  • How can the API be modified to be more straightforward to use?
  • What kind of enchantment helper methods should the API include for developers to use?
    • Should there be some kind of framework/a couple methods to help developers create custom enchantment tables? What are the potential use cases for that? Would enough people use that? Is that in the scope of QSL?

Closes #13

Vaerian avatar Jul 30 '21 08:07 Vaerian

How do you feel about including the ability to have default enchantments on items? The idea is there are many times where mods want to add say, inherent effects to their tools and armor, which can usually be represented the best through an enchantment (i.e. Fire Aspect). But adding an enchantment by default would remove the ability to further enchant in a table without removing said enchantment first. Default enchantments would be inherent to items, but not prevent the ability to random enchant in a table, and wouldn't be removable by grindstone or other means (debatable).

Platymemo avatar Jul 30 '21 20:07 Platymemo

Default enchantments would be inherent to items, but not prevent the ability to random enchant in a table, and wouldn't be removable by grindstone or other means

I believe this is already possible using some pretty basic mixins. I've not tried, but by looking at the code it seems like one would simply have to mix into ItemStack.hasEnchantments and have it return false if the item's enchantments nbt only contained the specific enchantment. After that it would be able to be enchanted in the table. To prevent it from being removed in the grindstone you'd have to mix into GrindstoneScreenHandler.grind and where the grindstone re-adds the cursed enchantments you'd also add a call that re-adds any marker enchantments. To make the enchantment come with any instance of the item stack I would just mix into the item stack constructor and set a default nbt tag.

I would be open to the idea of adding the idea of "marker enchantments." The restrictions I would recommend putting in place are that marker enchantments cannot be received from the enchantment table (that way you can't add a marker twice, or enchant something that shouldn't have the marker), they wouldn't be able to be removed in the grindstone, and they would not be specific to a given item (i.e. only some specific items could have the marker enchantment placed on them, but an enchantment would not be considered a marker based on the item it's applied to). I think adding a "default" enchantment for an item, however, is likely out of the scope of this module, that should be somewhere in the item's library as a way to add a default nbt tag.

ghost avatar Jul 30 '21 21:07 ghost

This is a question concerning API design. Enchantments are used in a wide variety of places such as random loot, mob equipment with random enchantments, villager tool trades, the enchantment table, and more. Ideally this API would give developers control over whether their enchantment can be added in any of those scenarios. The only issue is that all total that makes up about 6 different contexts in which enchantments can be used. It doesn't really make sense to create a method for every single one of those contexts so instead should we create a common interface for enchantment contexts and allow developers to check if the context is an instance of a specific type of context? The only issue I see with that is each scenario is a little bit different and it doesn't really make much sense for someone to have to write an if else statement with 6 conditions every time they want to write complicated functionality, but then again it's a little weird for someone to have to write implementations for 6 different methods. Additionally, making it a common interface might make it easier for developers to create their own enchantment contexts with special logic.

ghost avatar Jul 30 '21 22:07 ghost

You guys should add a way to make tools decide how much mending is affecting it, it would make sense since it's also related to enchantments

oliviathevampire avatar Sep 15 '21 17:09 oliviathevampire

You guys should add a way to make tools decide how much mending is affecting it, it would make sense since it's also related to enchantments

I wonder if this is really enchantment or item API. Kind of interesting thought. Though, the other thing to consider is: is this feature really widely needed?

LambdAurora avatar Sep 15 '21 17:09 LambdAurora

Though, the other thing to consider is: is this feature really widely needed?

For a mod I made, I wanted to let feather falling apply to helmets, and the extent of what I needed to go through to make this work was kind of insane just for how simple it seemed.. My method would also probably cause compatibility issues if other mods tried to do the same thing

I ended up creating a library called EnchantmentTweaker, which seems to cover a few ideas stated in the original post, so I think some of the code from my library may be of use for this idea 😃

omoflop avatar Sep 15 '21 18:09 omoflop

Is this still being worked on? I don't see any activity since July/August on @Vaerian's profile

OroArmor avatar Nov 12 '21 18:11 OroArmor

Succeeded by PR #291

EnnuiL avatar Apr 12 '23 01:04 EnnuiL