legobmw99 / Allomancy

Brandon Sanderson's Allomancy, now in Minecraft
https://minecraft.curseforge.com/projects/allomancy
GNU General Public License v3.0
23 stars 18 forks source link

Allomancy ore configuredfeatures are not registered #48

Closed TelepathicGrunt closed 3 years ago

TelepathicGrunt commented 3 years ago

Hello! Someone was running my mod called Blame with a small modpack and it seemed to have found that Allomancy's ConfiguredFeature form of its ores are not registered. This can be an issue for mod compatibility as under certain conditions, unregistered ConfiguredFeatures can basically prevent other mod's registered ConfiguredFeatures from spawning if in the same generation stage.

By that I mean, if mod A adds an unregistered CF to the ore generation stage and the biome's codec reaches it first, it will choke and basically nuke mob B's registered CFs afterwards. Here's a case where BetterCaves forgot to register their CF and caused several CFs from Oh The Biomes You'll Go to stop spawning in the world: yungnickyoung/YUNGs-Better-Caves#75

Here's a more detailed explanation of why this happens in the biome's codec: image

Specifically, when you call .withConfiguration on a Feature, you create a ConfiguredFeature. This is what should be registered to the WorldgenRegisties at mod init (you can do it in FMLCommonSetupEvent so you have your config ready too if it is needed).

Anyway here's an example from my mod RepurposedStructures of me registering all my ConfiguredFeatures. https://github.com/TelepathicGrunt/RepurposedStructures/blob/2b23538a37a474eaf560dd39baff705bfe22dab3/src/main/java/com/telepathicgrunt/repurposedstructures/modinit/RSConfiguredFeatures.java#L184-L185

I hope this helps!

From the log with Blame where it tried to figure out what unnamed configuredfeature is unregistered by parsing its json:


** Blame Report 1.9.1 **

This is an experimental report. It is suppose to automatically read the JSON of all the unregistered ConfiguredFeatures, ConfiguredStructures, and ConfiguredCarvers. Then does its best to collect the terms that seem to state whose mod the unregistered stuff belongs to.

Possible mods responsible for unregistered stuff:

allomancy:aluminum_ore allomancy:cadmium_ore allomancy:chromium_ore allomancy:zinc_ore

legobmw99 commented 3 years ago

Thanks for the in depth write up. This strikes me as something vanilla/forge should address by itself, but I'll include a patch in the next update.

legobmw99 commented 3 years ago

If possible, could you confirm that the above has resolved the problem? Thanks again!

TelepathicGrunt commented 3 years ago

from my quick glance, it seems like the biomeloadevent is still creating configuredfeatures which are not registered. https://github.com/legobmw99/Allomancy/commit/6d3031aadf273b68e98c9b7042d5242c55754d96#diff-b3096a60d9e317160b4eed99e7c3b3f311d2d47a7c6b825cc9ad3607af99b718R53-R66

The Configuredfeatures should be created and registeredbefore BiomeLoadEvent is fired and then the event uses that configuredfeature instance.

to be clear Feature.ORE.withConfiguration is what makes a configuredfeature. then each of the placements like .withPlacement(, .square(, and .func_242731_b all take the confgiuredfeature and wrap it in a brand new configuredfeature. The end result is configuredfeatures nested in each other with the outermost 3 being the count, square, and range and the inside is the actual ore. Only the very outermost one needs to be registered so

Feature.ORE
                .withConfiguration(new OreFeatureConfig(OreFeatureConfig.FillerBlockType.BASE_STONE_OVERWORLD, ore.ore_block.getDefaultState(), ore.vein_size))
                .withPlacement(Placement.RANGE.configure(new TopSolidRangeConfig(ore.min_height, ore.min_height, ore.max_height)))
                .square()
                .func_242731_b(ore.ores_per_chunk)

is what needs to be made before biomeloadevent and registered for every ore type

legobmw99 commented 3 years ago

Yes I had accidentally left some old code in the commit, the following commit should actually use the registered versions

TelepathicGrunt commented 3 years ago

Ah I see. I'll build a jar later and test to see if Blame doesnt report anymore unregistered configuredfeatures. I think it should be good

TelepathicGrunt commented 3 years ago

Just checked and yep, it's all good!

legobmw99 commented 3 years ago

Thanks again for the report! The forge docs really need a page on ore generation that mentions all these concerns