Fabric Structures v1 [1.16]

[ghost]

Full disclaimer at the top of this. I have no expectation that any of this code will actually make it into fabric with the way that it is. This is my first major contribution to the project and there's probably a lot of things wrong with it, but the start of this PR is simply meant to start the discussion on getting a public structures module for 1.16 and the new structure changes.

This PR addresses information from #661 and #123, here's a quick rundown to get you up to speed.

1.16 completely severed structures from features and added a new and slightly convoluted way of registering and making sure structures generate. This PR is meant to address some of those problems, specifically.

1. The ability to add structures to the generation step hash map through the use of an access widener (it's a private hashmap that is only ever used in a private method so unless we choose to mix into literally every place that hashmap is used this seems like the best bet)
2. Strongly recommending, but not forcing, developers to add their structure to the serialization hashmap to prevent the spam of errors in the log.
   1. Consequently by adding structures to this hashmap they are added to the /locate command
3. A FabricStructure class that standardizes which fields are responsible for registering what information for a structure and gives easier control to developers to control which name their structure is generated under and which generation step it is a part of
   1. This is done by strongly overriding a couple methods from StructureFeature to make the StructureFeature class itself the final call on generation steps, and to reference the StructureFeature itself in registering the generation step
   2. Softly overriding the default getName implementation to make it easier to include full identifiers to organize structures by mod and prevent conflicts than to not, while still providing all existing functionality and configurability for developers.

Overall I think the way things are currently implemented are the bare minimum to make things work, but they could easily be stricter if there's a need for a larger structures module.

A couple last things there are still a few unknowns in the structure equation. This PR doesn't handle anything associated to net.minecraft.world.gen.chunk.StructureConfig (the class formerly known as class_5311) but in my experience modifications to that weren't necessary to get structures to generate. That being said it's something that should be looked into so developers can control the spread between structures (if we're fitting with this style I'd recommend, once we figure out what's going on, having a method in FabricStructure that controls spread).

Additionally there are the very real concerns of #654 that should a mod be uninstalled that adds a structure, or the name of the structure changes it would potentially ruin the world. It would be awesome to provide a set of tools for developers, or a system behind the scenes that could control these kind of edge case situations where structures in mods no longer exist.

So just to recap this PR contains the bare minimum needed for mod authors to generate structures in the 1.16 prereleases but is meant as a way of starting the discussion about a larger structures module. Ideally said module would handle

• Adding structures to generation steps (this PR)
• Adding structures to serialization (this PR)
• Encouraging structure registration under identifiers (this PR)
• Adding structures to /locate (this PR)
• Organizing the chaos a little bit and making the StructureFeature themselves in control of things like their name and their generation step (this PR)
• Providing a tool to control the spread of structures (needs more looking into)
• Providing a tool to handle missing or renamed structures between versions (needs more looking into)

Hopefully this PR makes sense, once again this is my first major contribution so if it's totally wrong I'm sorry about that. Looking forward to discussing what else needs to be in a fabric structures module.

Completely separate to this, I'm hoping to go and make a PR to change some of the yarn mappings away from verbage that implies the union of Structures and Features (so like changing StructureFeature<?> to just Structure<?>) given that they are now officially severed. So if some of the language in this seems weird that's because I've been playing around with that kind of naming.

[ghost]

The issue seems to be the checkLicenseMain build task, I have no idea what that does, but the build looks fine otherwise

[liach]

Run gradle fabric-structures-v0:updateLicenses task. Also v0 is for modules from before the modularization, new modules should start with v1

[i509VCB]

licenseFormat should work to handle licenses

[ghost]

Is there a proper procedure for testing testmods because right now I'm just frankensteining it together

[ghost]

In theory everything should be working; however gradle was acting up for me so I wasn't able to run the testmod.

[i509VCB]

Is there a proper procedure for testing testmods because right now I'm just frankensteining it together

We don't really have a fully automated testing engine yet, so what you could attempt to automate (like in commands testmod) you could try. Otherwise you have to load the game to test

[ghost]

The checkstyle change commit handled most of the issues but it still doesn't like the package name. net.fabricmc.fabric.api.structure.v1 works but not net.fabricmc.fabric.api.structures.v1 should I just refactor it?

[i509VCB]

the checkstyle disallows plural package names. so structure would work

[i509VCB]

Hashmap is impl detail, should be closer to (don't quote it literally):

Suggested change

	* Registers a structure in the structure registry, the serialization hashmap, and generation step hashmap.
	* Registers a structure in the structure registry, it's generation step and serializer.

[ghost]

Should be better in 71b24d8

[TheBrokenRail]

@Earthcomputer figured out StructuresConfig! https://github.com/Earthcomputer/LibStructure

[ghost]

@TheBrokenRail @Earthcomputer

In reference to StructureConfigs I think I might have a solution. Using Enigma I figured out that the DEFAULT_STRUCTURES Immutable Hash map that makes it difficult to make custom Structure Configs is only ever used in 4 locations

1. In the constructor of actual instances of a StructuresConfig object which could easily be mixed into in order to add custom structure configs whenever an instance is constructed (Really kind of the only super important call between the four of these)
2. In the caves chunk generator type where it constructs a normal map that immediately after that has a call to add an element to the map, so again it would be really easy to mix into that method and add all of the custom structure configs
3. In one specific call when creating the flat world generator where it only accesses the village config specifically. We probably don't need to handle this case because
   1. The default vanilla implementation only accesses one specific value
   2. If developers are adding custom structures to the super flat world generator they are doing something out of the ordinary and are likely using way more mixins to achieve their goal so it doesn't really make sense to create a whole unique system to handle that one edge case
4. The addPreset method for the world presets screen which again makes its own map and then adds items to it so it shouldn't be hard to simply mixin there and add the custom structures configs. (I have a slight worry because it calls an unchecked get on the original hashmap which might throw an error but I haven't tested that yet)

So basically we should just be able to create a list, or a hashmap, or use the Structure registry in 3 mixins to add the custom configs wherever they need to be added. @liach mentioned above that iterating over a registry is not a super awesome idea so maybe we have a separate "FabricStructureConfigs" list that developers can add the configs to (and we can support in the helper methods) and then in each mixin do map.addAll(FabricStructureConfigs.stream().map(FabricStructure::getConfig)) or something like that.

I also want to add an injector onto StructuresConfig$method_28600 which seems to be the go to method to return the StructureConfig from the Structure object itself and basically just have it do the existing getOrDefault call but if the return is equal to the "blank" structure config and the Structure is an instance of FabricStructure then also try and call FabricStructure.getConfig for an added layer of redundancy.

Any objections with that plan?

[Earthcomputer]

Throwing an exception here rather than returning null avoids a nullability warning in the IDE.

[Earthcomputer]

I don't think there's any point of this class. I think we should enforce that mods use Identifiers, rather than only suggesting it. Given that, inserting into StructureFeature.STRUCTURES and StructureFeature.STRUCTURE_TO_GENERATION_STEP should be sufficient, and there's no need for this subclass.

[ghost]

I think we still need the class in order to force developers to define their generation step in the actual class. The way it currently works is the generation step is found from the hashmap and so adding the class to the hashmap is the only way to define the generation step and imo it’s much more elegant (and safe) to do it the other way around. Additionally when we implement structure configs I think it also makes sense to define the structure config as a method in the structure itself and then reference that in whatever our solution is

[Earthcomputer]

As well as my review, I should also point out that there are several things missing from this PR, two of which have already been mentioned:

1. StructureConfigs are missing
2. This PR does not add structures to superflat worlds. Although you say that mods shouldn't be doing this, I disagree. In fact, adding to superflat worlds is a great way to debug whether some types of structures are generating correctly. Also your point about the superflat stuff only applying to villages is incorrect.
3. This PR does not correctly add jigsaw structures to StructureFeature.field_24861. I am not entirely sure why this is necessary yet, but it contains the jigsaw structures and the jigsaw generating code references this field.

My library handles all three of the above, although one may argue that the implementation of the first one is a little messy and may not get through fapi bikeshedding. I also have not thought of a way to allow for different StructureConfigs to be used in different vanilla dimensions. As an aside, this is what's going on with the ruined portals in the caves chunk generator type - it's not a new structure config, it merely replaces an existing one.

I look forward to further discussions on Discord as to how best to implement these features in fapi. For those who have not seen it yet, it may be worth checking out my library for its implementation.

[frqnny]

I believe that field is for structures to modify the land around them, so it can fit right into the villages.

[frqnny]

I do agree with earthcomputer, I would like to see the changes Earth stated.

[ghost]

Okay I feel it might be wise to have someone else take the lead on this, my preliminary observations were just that and I don’t have an in depth understanding of all of the details. Not sure what that process will look like but I don’t think I’m the right person to continue on this particular job.

[TelepathicGrunt]

StructureFeature.field_24861 is indeed what vanilla uses for adding land around the base of Pillager Outposts and Villages which is incredibly useful for modded jigsaw structures.

[shartte]

Yes, structure configs are essential. I've added a few javadocs in yarn here: FabricMC/yarn#1552

If you don't have structure configs, it'll try to spawn your structure every chunk...

p.s.: In case anyone is porting from earlier versions. The default was spacing=32, separation=8 in 1.15.

[TheBrokenRail]

It looks like spacing config is now data-driven in 1.16.2, so it might be easier to add this for 1.16.2 instead of 1.16.

[ghost]

I haven’t looked at it too in depth but it seems like the end goal of all of this refactoring is to make everything data driven, do you think we’ll even need this anymore?

[Earthcomputer]

Spacing config has been data-driven since the 1.16 snapshots, nothing new there. What this API is needed for is adding new structures (which the vanilla API is horrible for), rather than configuring vanilla ones which is possible with datapacks.

[Earthcomputer]

i5 has made a fix for the vanilla bug: #654 (comment)

[ghost]

It seems to me that given the recent changes to world gen in 1.16.2 and the fact that this PR did not address the entirety of structure functionality to begin with that an alternative solution ought to be developed and that this PR is no longer a path that should be pursued. With that being said I'll give it 30 days to see where the conversation goes before either deleting the PR or passing it off to someone else who wants to head this project.

[frqnny]

This needs to be closed.