Biome Modification API - #1097
Conversation
ghost
left a comment
There was a problem hiding this comment.
I think in terms of actual code it looks go to me. It might be benefited from some additional comments and documentation in some of the mixins, but in the grand scheme of things that's pretty minor.
… top-level convenience method to add new spawns.
|
@vaerian I tried to address the docs on RegistryOpsMixin (which is arguably the most important one) and on the one that adds the modified biome tracker. The accessors I'd leave undocumented since they're just added getter/setter soup. |
|
|
||
| OverworldBiomes.addContinentalBiome(BiomeKeys.END_HIGHLANDS, OverworldClimate.DRY, 0.5); | ||
|
|
||
| BiomeModifications.create(new Identifier("fabric:test_mod")) |
There was a problem hiding this comment.
I would expand the tests a bit, I would atleast add an ore or something as well.
There was a problem hiding this comment.
Please see the unit tests I linked in the PR description, I don't want to duplicate those in the test mod until it's clear whether they'll go through or not.
There was a problem hiding this comment.
Those are awesome, they would be really great to have in. They required a loader change didnt they?
There was a problem hiding this comment.
The commit contains more a "proof of concept" for how to do a loader-change, by placing a class in the loader-package to get around the package-visible accessibility of some loader classes. It obviously can't stay like that. Maybe we can convince player that it is worth supporting, then I'd try to make a loader PR (it shouldn't really be too hard, essentially we'd need a third KnotLauncher that doesnt invoke any entrypoints and uses more or less a dummy GameProvider, which is what that proof of concept does).
There was a problem hiding this comment.
I do still think there should be few more examples/tests here, atleast add an ore or something.
modmuss50
left a comment
There was a problem hiding this comment.
This is looking really solid, would be nice to have a few more tests in the test mod nothing great is required. We can look into getting the full test suite in later?
|
|
||
| OverworldBiomes.addContinentalBiome(BiomeKeys.END_HIGHLANDS, OverworldClimate.DRY, 0.5); | ||
|
|
||
| BiomeModifications.create(new Identifier("fabric:test_mod")) |
There was a problem hiding this comment.
I do still think there should be few more examples/tests here, atleast add an ore or something.
|
Removed the exposed integer-based ordering and instead used phases to represent the constants that were on the api class before. |
|
Does not work for StructureFeature, as they need to have a StructureConfig added to their ChunkGenerators StructuresConfig, else the call to structuresConfig.getForType returns null and the StructureFeature is skipped. |
|
@Stuff-Stuffs Did you register you structures using fabric api's structure api? It should add the structure configs to a special map that all chunk generators should then inherit from |
|
I did, however my problems have dissipated since invalidating caches, problem fixed. I guess the caching messed something up. |
modmuss50
left a comment
There was a problem hiding this comment.
Looking really good now, still wondering if it would be a good idea to have a BiomeSelector that uses Biome.Category ?
|
I really hope this can be merged soon... |
|
Agreed MCrafterzz. I really want to drop the current system I have for adding to biomes. And having this PR means I can make a structure tutorial for Fabric without needing hacks that could cause issues or have downsides |
Player3324
left a comment
There was a problem hiding this comment.
LGTM, thanks! I believe the biome selector thing is worth doing after all..
* Biome Modification API * Improved docs on Mixins. * Added convenience methods to select biomes by mob-spawns, and added a top-level convenience method to add new spawns. * Checkstyle fixes. * Replace a reference to DRM with DynamicRegistryManager * Replaced integer order with phase-based ordering. * Changed to @deprecated * Checkstyle fix * Added category selector.
|
BUT HOW DO WE ACTUALLY USE IT |
|
@SuperPlantVoiderOriginal Fabric's APIs generally have test mods on the GitHub repo that shows an example usage of the API. For the Biome Modification API, here's the test mod for that which you can look at for an example. Note, this API is only available through code so there is no way to access this modification through JSON files only unlike a few other APIs. If you have further questions, please go onto the Fabric Discord and ask there as someone will be willing to help you out. |
This builds on the previousy biome API PR to add a way for modifying existing Biomes. This is needed since 1.16.2 made Biomes immutable, and introduced several copy-steps which make it harder to simply modify the Biomes in BuiltInBiomes and be done with it.
This API aims to add a more generic way of allowing ordered modifications, while building some heavily used convenience methods on top of that more generic API (i.e. adding features, and structure should be easy, while still allowing more wide-spread mods).
Implementation Notes
Most important classes:
BiomeModificationsis the entry point for any form of modificationBiomeSelectorscontains commonly used selectors such asall biomes in the overworld,all biomes in the nether,all biomes in a given list, etc. (selectors can be combined like standard Java predicates)BiomeModificationContextis the object given to the actual modifier to add/remove/change the biome as neededExample usage (for the simple ore generation case, with some extra config bonus):
Example usage (for the simple custom structure case, with some extra config bonus):
Example usage (biome-specific mineshafts using biome-based selectors and multiple modifiers):
Example usage (biome-specific mineshafts using context-sensitive modifier):
Example usage (making an entity spawn in all biomes that also spawn sheep):
Example usage (replace sheep spawns with custom sheep in all overworld biomes below a given temperature as long as they're not oceans, and inherit their spawn settings - requires a custom accessor to get all properties of existing spawn entries):
For the curious, the tests are currently here: b02d6a7