search

QuiltMC / quilt-standard-libraries

A set of libraries to assist in making Quilt mods.
Apache License 2.0
152 stars 85 forks source link

The OverworldBiomeParameters Non-default Registry Bug™ #292

Closed sylv256 closed 1 year ago

sylv256 commented 1 year ago

What is this?

Hello. It has been brought to the attention of a few Quilt mod developers that a terrible bug plagues our blessèd lands. I am here to provide the details of the bug. Feel free to rename or edit this, since I'm not very good at making issues. (Relevant issue: https://github.com/TerraformersMC/Biolith/issues/1)

???

In essence, the bug applies where one adds a non-default Biome to the OverworldBiomeParameters class or uses any modded Biomes before the official Mojank™ datapack registration (which apparently happens after said parameters are initialized, i.e., long after the net.minecraft.world.gen.BiomeData::bootstrap[1] or net.minecraft.world.biome.source.util.OverworldBiomeParameters::addBiomesTo methods have been invoked).

Is that it?

Kind of. Actually, what's odd is that this issue never occurs on Fabric. A small quote from Falkreon: A series of messages from Falkreon detailing the possible culprit of the bug. Quote, "Hmm, QSL's organization system is pretty opaque to me. Artifacts are named 'dimension-3.0.0-beta.(...)' instead of 'QSL-dimension-3.0.0-(...)' so they won't sort together on a dependency list. And, it's not obvious what will be a QSL component as opposed to a new Minecraft dependency. I think though the behavior might be happening at the end of MinecraftServer's constructor, the relevant code being at (see note #2). This is mixin'd at return of MinecraftServer::<init>. I don't see the unfreeze from here, but it looks a lot like a Fabric organizational plan­­—hit it late, after it's been frozen, then unfreeze it, apply modifiers, and then re-freeze it. QSL likes to hit it in the right place: before it's been frozen. I don't know though, this is just my first glance at it." A later quote from Falkreon: Looks like it's already there, I appear to be wrong though. Where I point out doesn't seem to be where the real problem happens. The issue appears to occur due to a difference in Quilt's handling of registry. As shown in the stacktrace in the logs below, the exception is likely thrown due to a net.minecraft.registry.VanillaDynamicRegistries::createLookup call in org.quiltmc.qsl.tag.impl.client.ClientTagRegistryManager::<clinit>, but moreso due to a net.minecraft.registry.RegistrySetBuilder$BuildState::reportRemainingUnreferencedValues call by net.minecraft.registry.RegistrySetBuilder::build. Furthermore, the suppressed exception detail reads, java.lang.IllegalStateException: Unreferenced key: ResourceKey[minecraft:worldgen/biome / wwizardry:forgotten_fields]. This suggests that an unregistered biome wants to be referenced, but fails.

Can I see some logs?

Yes.

Notes

[1] - This issue uses Quilt Mappings version 1.19.4+build.7.

[2] - org.quiltmc.qsl.worldgen.biome.impl.modification.BiomeModificationImpl::finalizeWorldGen

EnnuiL commented 1 year ago

This is the exact same issue as the one we dealt on porting the nether biome injections to 1.19.4, and once being reached out, I did attempt to explain it; Lemme reiterate: Y'all are injecting stuff too early!

Fabric's nether biome injection (which was initially ported to QSL, which uhhhhhh, was a terrible idea) is injecting the biomes at the wrong moment, and that ends messing up Quilt Tag API's attempt to read through the registries; This means that yeah, Fabric is doing things wrong, it's just that it's not exploding due to nobody poking at the bomb (which reminds me of the whole block/item content registries situation!)

The solution is simple: inject at the perfect moment, which for us, turned out to be this for nether biomes, with the target for overworld biomes likely being on the biome sources that they use (which yeah, i doubt it's the multi-noise one); Do note that there are two parameter lists for overworld, one for the baseline and another for it plus 1.20 content;

Unless we end determining that what Quilt Tags API is doing is wrong? Yeah, I don't think we can reasonably fix it on our sides, because yeah, really, its usage of VanillaDynamicRegistries.createLookup() is the only cause of the issue, not any biome injection shenanigans done by us, and yeah, I don't really see any reason for that method to be left broken, which I could see another mod reasonably using it too; I'd like @LambdAurora's thoughts on this though, because I might have found the issue, but it doesn't mean i fully understand it!

gniftygnome commented 1 year ago

(Thanks for discussion in the Quilt discord BTW.) FWIW I have a general idea how I can work around this in Biolith but I have not had time yet to validate it. I basically intend to do what Quilt is doing (what some other biome mods do) -- add the biomes later, in [YM] MultiNoiseBiomeSource.getBiomeEntries(). This does not seem as clean to me, but as far as my concerns go, primarily it's just whether the same approach will work for Fabric or I have to bifurcate the codebase. Also also, Quilt's mixin is not super compatible with other mixins trying to do the same thing in the same way, since it may cancel. I am hoping to work around that for Nether biomes by using MixinExtras' @WrapOperation mixin to target the same anonymous function Quilt does.

LambdAurora commented 1 year ago

Unless we end determining that what Quilt Tags API is doing is wrong?

I might have found something that may appease this error. Realistically client/fallback biome tags should not be accessible outside of a dynamic registry context, so I could simply not make the hardcoded values load, which might be a single line fix in Quilt Tags API.