Biome Modification API does not apply changes to datapack biomes.
TelepathicGrunt opened this issue ยท 10 comments
My mod Repurposed Structures utilizes the Biome Modification API for all of its structures. However, when quickly testing edge cases for my configs to make sure my structures are applied to the right biome, I used a datapack and found my changes never gets applied. This affects any biome overwritten by the datapack's json biome and also affects any brand new biome made by the datapack.
To reproduce, download Repurposed Structures and this datapack and create a new world with the datapack on.
https://www.curseforge.com/minecraft/mc-mods/repurposed-structures-fabric
data.zip
The Overworld, Nether (these two dim have all their biomes overridden by datapack), and Custom Dimension (this dim has the custom brand new biome called new_plains) is what we are going to look at. If the Biome Modification API worked as intended, the following result for the locate command should be the following:
OVERWORLD
- /locate stronghold : failed
- /locate repurposed_structures:stronghold_stonebrick : found
- /locate repurposed_structures:stronghold_nether : failed
NETHER
- /locate stronghold : failed
- /locate repurposed_structures:stronghold_stonebrick : failed
- /locate repurposed_structures:stronghold_nether : found
CUSTOM
- /locate stronghold : failed
- /locate repurposed_structures:stronghold_stonebrick : found
- /locate repurposed_structures:stronghold_nether : failed
However, in reality, we find that the API does not do any changes and the result of the locate command is actually this with the datapack on.
OVERWORLD
- /locate stronghold : found
- /locate repurposed_structures:stronghold_stonebrick : failed
- /locate repurposed_structures:stronghold_nether : failed
NETHER
- /locate stronghold : failed
- /locate repurposed_structures:stronghold_stonebrick : failed
- /locate repurposed_structures:stronghold_nether : failed
CUSTOM
- /locate stronghold : found
- /locate repurposed_structures:stronghold_stonebrick : failed
- /locate repurposed_structures:stronghold_nether : failed
Biomes done by code seem to be fine. Next to test is whether other mods that make their own biomes by JSON within themselves also suffer from this issue with my mod using the API.
Update: it does indeed impact any mod using JSON biomes. So Repurposed Structures cannot add structures to any mod with JSON biomes as well. Rip
@maityyy Json biomes is commonly used in mods because it is easier to do than to make code based biomes which is why this is a major issue.
Here, searching creature_spawn_probability will reveal 22 pages of mods using json biomes that are listed on github
https://github.com/search?l=JSON&q=%22creature_spawn_probability%22&type=Code
Sounds awful. However, are json biomes used by mods? For some reason, even Mojang doesn't use them yet.
Chiming in to say that a friend has hooked into the exact same place in RegistryOps on Forge to do a biome modification api test for Forge.
https://github.com/FabricMC/fabric/blob/1.17/fabric-biome-api-v1/src/main/java/net/fabricmc/fabric/mixin/biome/modification/RegistryOpsMixin.java
What they found is their hook works and they are able to add to JSON biome perfectly fine. This means the issue is not that mixin at all. Instead, the issue here is inside of the Biome Modification API itself. Something about it is not doing the modifications correctly for JSON biomes.
At a quick glace and judging by the comment, maybe this part is the cause of the issue?
It would make sense. If the RegistryOps fires multiple times but it resets the JSON biome each time, then this line is going to set the biome the first time, save it to the set, get the biome from the set when the RegistryOps fires a second time, and skip the JSON biome even though it is a new instance of the biome that does need the modifications
I guess an option is to change Set<RegistryKey<Biome>>
to Set<Biome>
and only skip the biome instances modified instead of saving registry keys. But this is a memory leak as new biome instances are created each time you enter a world and so, you could eventually get an out of memory error if you enter/exit worlds thousands of times lol. Tricky issue to solve....
though, this set may not even be necessary. A test should be done without the set being used at all and a check to see if the features are truly being added twice or not.
Resolved by #1463.
Leaving a small update to this issue report, I am noticing users are starting to make use of datapack biomes quite a lot now to change biome colors or to remove stuff from biomes. However, I had come across people who don't want to track down every modded feature and structure done by BiomeModification api in order to re-add it to the biome json files since the api doesn't work. When I get time in the far future, I may take a look at BiomeModification api and see if I can get it working for json biomes due to increasing requests and inquiries about if my mod works for datapack biomes.
Or not... the mixin is bad either way. I will need to expand the test mod to test this fully.
Spent a good few hours with a debugger trying to figure it out, and still not sure whats up.
BiomeModificationContextImpl is being applied to all biomes as far as I can see, but somewhere in the DFU mess? the changes are not making it all the way into the biomes GenerationSettings or the
I have tried changing the injection points to match those of the registry sync module (Used to remap the same registry) without any success.
I think the next stage is to work downwards from GeneratorOptions/BiomeSource , this seems to be related but ive had enough for now.