Darkhax-Minecraft / BotanyPots

Adds some flower pots that can be used to grow various crops.
GNU Lesser General Public License v2.1
92 stars 71 forks source link

[BUG] Botany Pots not recognising seemingly random items as soils #373

Closed Bentcheesee closed 4 months ago

Bentcheesee commented 4 months ago

Minecraft Version

1.20.4

Mod Version

16.0.3

Mod Loader

Fabric

What environment are you running the mod in?

Client

Issue Description

Materials seem to randomly not work correctly as soils. Video explanation/showcase:

https://youtu.be/u9YYZG-rHLY

Darkhax commented 4 months ago

Hello, could you please share some more info about how you're creating the crops/soils?

Bentcheesee commented 4 months ago

So using CraftTweaker, I'm able to add or remove recipes from the game, sort of like a more advanced data pack. That being said, the bug in particular is a little bit more different than I thought at first due to some testing and messing around.

In the video linked in the original report I show various blocks that show up in the recipe via a item tag and REI due to me creating the recipe, what I didn't show was all the code to make that recipe appear. Apparently, the materials that did not work was due to me not removing said materials as soils from the baseline mod recipe supply. What appears to be the case, is when a material shows up under two soil recipes at once, it causes that material to not work for either soil recipe.

Darkhax commented 4 months ago

What appears to be the case, is when a material shows up under two soil recipes at once, it causes that material to not work for either soil recipe.

Yes, this is the intended behaviour. Any item can only belong to one soil and adding it to more will cause issues, just like crafting recipes or furnace recipes. You should be using the category system for controlling compatibility rather than item tags. The current categories are a bit broad, but you can replace them with more specific categories if that's what you're going for.

Bentcheesee commented 4 months ago

Out of curiosity, why would this be intended?

My argument against it lies in the fact that you could have the same materials have different growth speeds dependant on what crop you're attempting to grow.

For example, Crop X would grow at 1.1x on Material A using Recipe A while Crop Y would grow at .75x on Material A using Recipe B while Crop X grows at .75x on Material A using Recipe C. Crop Y grows at 1.1x on using Recipe D

Darkhax commented 4 months ago

It's intended for performance reasons. Currently the soil lookup code will scan the registry and stop on the first match. So if you have 500 soils and the 8th one matches it will only try the first 8 soils. Supporting multiple matches would require trying all 500 soils even if only one will match. This would make the performance of the lookup code get slower with each new soil added to the game just to support the subset of scenarios where multiple matches may exist.

This sounds like an XY problem though. If you want soils that have different modifiers for different seeds that's something that could be added as a property of the soil itself without adding conflicting entries. This has been a planned feature for a while now but it keeps getting pushed back by things like MC updates or adding support for new mod loaders.

To help illustrate the idea, here is a mockup of what the JSON would look like to add growth modifier overrides. In this case I am using an item but you could just as easily use a tag there if you prefer.

{
  "type": "botanypots:soil",
  "input": {
    "item": "minecraft:grass_block"
  },
  "display": {
    "block": "minecraft:grass_block"
  },
  "categories": [
    "dirt",
    "grass"
  ],
  "growthModifier": {
    "default": 1,
    "overrides": [
      {
        "input": {
          "item": "minecraft:poppy"
        },
        "modifier": 2
      }
    ]
  }
}