MagmaGuy / BetterStructures

Adds structures to Minecraft worlds using WorldEdit!
GNU General Public License v3.0
24 stars 11 forks source link

Custom Enchants on Newest Version are insane. #26

Open Exanthiax opened 11 months ago

Exanthiax commented 11 months ago

Please add support for EcoEnchants and other custom enchant plugins.

On the latest version with procedurallyGenerateEnchantments, items spawn with absolutely any enchantment on it, even ones that have discoverable disabled in EcoEnchants. This wasn't an issue before the latest version, things spawned alright, but I loaded up after updating and I have items generating with 30+ enchants on them, majority are ones we have disabled from being found in the world.

I've disabled the setting but some direct support for this would be good. https://github.com/auxilor/ecoenchants <-- This is the github for Auxilor's EcoEnchants, there is the API for it, this should be good to get this working for anyone that uses this.

Exanthiax commented 11 months ago

Adding: procedurallyGeneratedItemSettings: is nice, but when you have lots of custom enchantments, I'd hope a better solution can be found, as 13k lines of enchant settings is really not a good experience for the user. I am paper 1.20.1

Exanthiax commented 11 months ago

Adding: we also wouldn't have known that we should disable this as "fixed loot tables" is very vague and had to search Github to see what got added, just to make sure that it was this update that caused this.

Edit: by "fix loot tables" I was referring to the update name on spigot, hard to tell what actually got added to the plugin without diving into the code

MagmaGuy commented 11 months ago

By default there are 2749-ish configuration lines, if you are not running something like EcoEnchants. BetterStructures reads from all of the vanilla Minecraft enchantments and makes sure to generate a configurable table. It sounds to me like EcoEnchants is spoofing Minecraft keyed enchantments, which would make them show up in the list. That's not intentional, I can't do anything about other plugins faking vanilla Minecraft enchantments short of me individually programming each individual enchantment, and making sure they are valid for the user's Minecraft version, which I would rather not. Besides, if I did, people would then immediately start asking for EcoEnchants compatibility. You can ask EcoEnchants to stop injecting enchantments into the Minecraft-reserved namespace if you want to. Also, I don't even know by fixed loot tables, that's not a valid current setting anywhere.

WillFP commented 11 months ago

Dev of EcoEnchants here - couple things:

MagmaGuy commented 11 months ago

Dev of EcoEnchants here - couple things:

  • I have to use the minecraft namespace for compatibility / API reasons
  • If you want to check if an enchantment is an EcoEnchant rather than a vanilla enchantment, you can just do enchant instanceof EcoEnchant
  • If you want to check if an EcoEnchant is discoverable, you can also just do this:
if (enchant instanceof EcoEnchant ecoEnchant) {
  return ecoEnchant.isDiscoverable();
}

And if you're using Paper, then Enchantment#isDiscoverable is in the default API, no integration needed

First off thanks for dropping by.

That being said, do you have a suggestion for what I see as the core issue here, that being that people simultaneously wanting EcoEnchants compatibility, but not wanting it to be as common as it currently is?

If I understand correctly, discoverability is one potential setting for this, but would this be a full fix for the issue as described by the user or is there a need to change the chance for EcoEnchants enchantments to be on an item / have specific chances per enchantment and, if so, based on what?

For reference, in case you have not seen what the configuration looks like, this is what a default entry for a procedural enchantment looks like, all automatically generated based on Enchantment fields:

procedurallyGeneratedItemSettings:
  golden_sword:
    bane_of_arthropods:
      minLevel: 1
      maxLevel: 5
      chance: 0.2
    looting:
      minLevel: 1
      maxLevel: 3
      chance: 0.2
    smite:
      minLevel: 1
      maxLevel: 5
      chance: 0.2
    unbreaking:
      minLevel: 1
      maxLevel: 3
      chance: 0.2
    vanishing_curse:
      minLevel: 1
      maxLevel: 1
      chance: 0.2
    mending:
      minLevel: 1
      maxLevel: 1
      chance: 0.2
    sharpness:
      minLevel: 1
      maxLevel: 5
      chance: 0.2
    knockback:
      minLevel: 1
      maxLevel: 2
      chance: 0.2
    fire_aspect:
      minLevel: 1
      maxLevel: 2
      chance: 0.2
    sweeping:
      minLevel: 1
      maxLevel: 3
      chance: 0.2

Note that the chances here represent a 20% chance of the enchantment being on an item, with the system then randomizing the level within the bounds set between minLevel and maxLevel.

The issue that I see here is that in a potential scenario where people do have 100+ valid enchantments for a specific material, even if the discoverability is applied and the enchantments are culled down to 30, with the current settings 20% of 30 is 6 so there would be a good chance of the amount of enchantments on that material would be around 6 on average.

Is there a way around this?

Exanthiax commented 11 months ago

Maybe this will help with the question. Within EcoEnchants, we can choose the rarity and type of each enchant, and then we can set the chances of getting these enchants in the rarity file, here is an example:

  - id: uncommon
    display-name: "Uncommon"
    table-chance: 16
    minimum-level: 5
    villager-chance: 9
    loot-chance: 12

Along with this, we can also choose the max amount of enchants per-type allowed on an item. This max number is also respected for loot generated using EcoEnchants, here is an example from this file:

  - id: tool
    format: "&6"
    limit: 8
    high-level-bias: 0
    no-grindstone: false

Maybe this helps, maybe this doesn't, but might give some insight into the configurations already done on EcoEnchants side

MagmaGuy commented 11 months ago

Maybe this will help with the question. Within EcoEnchants, we can choose the rarity and type of each enchant, and then we can set the chances of getting these enchants in the rarity file, here is an example:

  - id: uncommon
    display-name: "Uncommon"
    table-chance: 16
    minimum-level: 5
    villager-chance: 9
    loot-chance: 12

Along with this, we can also choose the max amount of enchants per-type allowed on an item. This max number is also respected for loot generated using EcoEnchants, here is an example from this file:

  - id: tool
    format: "&6"
    limit: 8
    high-level-bias: 0
    no-grindstone: false

Maybe this helps, maybe this doesn't, but might give some insight into the configurations already done on EcoEnchants side

Well this seems like an approach vector, can you clarify a few points here? Would basing my own chance based on table-chance or loot-chance potentially achieve something here? Are those chances weighted or how do they work?

I see you have an ID that says "tool" there, I don't know if I can use that since my own system is based on materials, or am I just not looking at the full picture there?

Ideally if you could tell me "Pull loot-chance from the API and divide by 10 for the chance and check if the total amount of enchants doesn't go over CoolAPI.checkItem(itemStack) or enchantCount < CoolAPI.maxEnchants(material)" that would essentially solve this conundrum.

Exanthiax commented 11 months ago

Auxilor would be best to comment on if its weighted, or the third paragraph. With ref to materials and the ID for the enchant type. When we create an enchantment, we specify the targets that the enchant applies to, here is that section from an enchantment:

display-name: "Unbreakable"
description: "Your tool becomes unbreakable"
placeholder: "%level%"
type: tool
targets:
  - axe
  - pickaxe
  - shovel
  - shears
  - hoe
  - flint_and_steel
conflicts:
  - mending
  - toughness
rarity: veryspecial
max-level: 1

What comes under each target is specified in targets.yml

targets:
  - id: pickaxe
    display-name: "Pickaxes"
    slot: hand
    items:
      - "*wooden_pickaxe"
      - "*stone_pickaxe"
      - "*iron_pickaxe"
      - "*golden_pickaxe"
      - "*diamond_pickaxe"
      - "*netherite_pickaxe"

By pulling the enchant data it should also be possible to pull the targets/materials?