fabric
fabric copied to clipboard
Restore vanilla's gametest structure loading
If a structure cannot be found, we now fall back to vanilla's codepath, rather than throwing an exception. This offers the following advantages:
- Makes it easier to use game tests in multi-loader setups, as the structure path is consistent across loaders.
- As structures are now looked up from
testStructuresDirectoryName
, it's now possible to store structures in plain text (.snbt) rather than their binary form (.nbt).
it's now possible to store structures in plain text (.snbt)
-
The existing code only supported using snbt files.
-
With this change a missing structure will throw a misleading error about the fallback location.
-
The testmod should be expanded to cover this.
I wonder if it would be better to offer an API that allows mods to customise how the structures are loaded? The vanilla location for test structures is less than ideal IMO, allowing mods to have full control over this seems like an even better solution to me?
The existing code only supported using snbt files.
Ahh, you're entirely right. I think I had registered this at some point, but forgot by the time I came to write the commit message. Apologies!
With this change a missing structure will throw a misleading error about the fallback location.
Would you prefer a more complete implementation which tries the three paths and lists the paths it tried?
I wonder if it would be better to offer an API that allows mods to customise how the structures are loaded? The vanilla location for test structures is less than ideal IMO, allowing mods to have full control over this seems like an even better solution to me?
Initial disclaimer here that this may be in part just a documentation/understanding issue. I'm fairly sure I understand how the vanilla/Forge gametest workflow works, but still trying to grok what Fabric has (and hasn't) changed.
While vanilla's default location definitely isn't perfect, testStructuresDirectoryName
isn't final, which means mods can overwrite it to point to a more sensible location. More crucially, /test export
saves to this directory, which makes the test workflow much more convenient than having to shuffle files about after exporting.
My concern with an API is that once you start looking in custom locations, you break the symmetry between the /test import|export
commands and the actual gametest structure loading. I'm not sure - happy to implement whatever API if it is wanted though!
More crucially, /test export saves to this directory, which makes the test workflow much more convenient than having to shuffle files about after exporting.
This is a great point, the current solution does add a bit more friction for developing these tests. The easier they are to add, the more likely people are going to adopt them. (I dont think many people use them right now)
Its been a little while since I looked at this stuff so I think I'll need to familiarise my self before I can give a fully informed suggestion.
Looking at this patch again, it occurs to me that falling back to vanilla behaviour isn't correct either, as the template name now contains the mod id, so this will look up files in the form modid:test_name.snbt
instead.
I wonder if the following set of changes is a little more preferable:
- Make this mixin now search in
Paths.of(testStructuresDirectoryName, structureId.getPath() + ".snbt")
and then inside the datapack. - Support setting
testStructuresDirectoryName
via afabric-api.gametest.structure-dir
system property.
Mods can then set this property to src/testmod/resources/data/$modid/gametest/structures
inside their Gradle config. This is a little clunky, so I'm not too sure about it, but it does preserve. the existing layout and behaviour but with the existing advantage that the /test
commands work as expected.
I believe the proper way, 1.19+, is to register StructureTemplateManager.Provider
instead of Mixin.
I guess the problem with StructureTemplateManager.Provider
is that it also doesn't provide the paths it has tried. I don't know how much that matters?
Superseded by #2801.