quiqueck / BCLib

A library mod for BetterX team mods, developed for Fabric, MC 1.16.4+
https://modrinth.com/mod/bclib
Other
25 stars 24 forks source link

Only use fabric's mining level api to enforce ore mining levels #182

Closed kira-bruneau closed 4 months ago

kira-bruneau commented 4 months ago

Fixes mining level incompatibilities for mods that don't derive tools from TieredItem (eg. Alloygery #86, Hephaestus, ...)

Also renames #c:needs_netherite_tool to #fabric:needs_tool_level_4 to make netherite-level ores compatible with fabric's mining level api. All other mining levels tags were already compatible.

This fix only applies to 1.20-1.20.4, fabric's mining level api was removed in 1.20.5: https://github.com/FabricMC/fabric/pull/3664

kira-bruneau commented 4 months ago

Oops I also noticed to fully fix #86, this would be backported to 1.19 too. I'm not sure if I should rebase this or open a new PR though - there are merge conflicts between 1.19 and 1.20.

kira-bruneau commented 4 months ago

Oh, I also noticed that BetterEnd ores don't specify a mining level, and the default mining level of 0 causes the MiningLevelManager to throw an exception: https://github.com/FabricMC/fabric/blob/1.20.1/fabric-mining-level-api-v1/src/main/java/net/fabricmc/fabric/api/mininglevel/v1/MiningLevelManager.java#L74.

I updated the default to Tiers.STONE.getLevel() instead. It looks like before this it was defined to mine those ores with an empty hand, which I don't think was intended?

I also noticed that the null checks for tool were unnecessary, because LootParams.Builder.getParameter would throw if it was null:

public <T> T getParameter(LootContextParam<T> lootContextParam) {
  T object = this.params.get(lootContextParam);
  if (object == null) {
     throw new NoSuchElementException(lootContextParam.getName().toString());
  } else {
     return object;
  }
}