Extensions to datagen API for model generation #2754

62832 · 2022-12-12T22:57:34Z

For a while now, I've been meaning to extend Fabric's datagen API in order to provide support for more complex data generation, particularly for block and item models. Ideally, beyond block/item models, extra providers could also be implemented for other data such as particles and sounds.

At present, the datagen suite within vanilla itself is severely limited for models, providing not even half of the JSON properties that are actually supported by models such as individual elements and item-specific overrides. This PR aims to provide various custom builders to facilitate the generation of these properties as additions to vanilla's own model classes via interface injection. These builders mostly follow the spec provided by the following Minecraft Wiki page.

Currently, this is still a very early draft. Among other things, I have not figured out a particularly adequate method for actually building models with these new properties that doesn't just involve toying around with the various models and texture mappings used by the existing vanilla generator methods, as can be seen in the test mod.

fabric-data-generation-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/FabricModel.java

Technici4n

A few misc comments.

...neration-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/builder/DisplayBuilder.java

fabric-data-generation-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/FabricModel.java

...neration-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/builder/ElementBuilder.java

...-generation-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/builder/FaceBuilder.java

62832 · 2022-12-16T17:26:08Z

I've marked this PR as ready for review as I am more or less content with the model builders themselves in their current form, however I am still open to suggestions on how to process might be better simplified with any specific generator methods. I would hope that the ability to reuse builders and re-generate resulting models should help keep things clean for mods with a greater amount of models with variably-complex properties.

...1/src/main/java/net/fabricmc/fabric/api/datagen/v1/model/FabricBlockStateModelGenerator.java

modmuss50

Looks like a great start, I am not too familar with custom models so im unable to review that too well.

Please let me know if you dont agree with anything ive pointed out.

...-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/model/property/RotationBuilder.java

...n-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/model/property/DisplayBuilder.java

...tion-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/model/builder/ModelBuilder.java

...n-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/model/property/ElementBuilder.java

...tion-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/model/builder/ModelBuilder.java

modmuss50 · 2022-12-20T16:03:47Z

fabric-data-generation-api-v1/src/main/java/net/fabricmc/fabric/mixin/datagen/ModelMixin.java

+		displays.clear();
+		elements.clear();
+		overrides.clear();
+		guiLight = null;
+		ambientOcclusion = true;


Why is this needed?

Since these are mutable fields, any Model for which they're set will retain these properties even when it isn't intended. This was a concern at an earlier stage when standard models such as Models.GENERATED would retain these properties, though I'll try to give this a further look and see if it still applies.

modmuss50 · 2022-12-20T16:05:07Z

...on-api-v1/src/testmod/java/net/fabricmc/fabric/test/datagen/DataGeneratorTestEntrypoint.java

+			blockStateModelGenerator.buildWithSingletonState(BLOCK_WITH_CUSTOM_MODEL_1, customModel);
+
+			customModel.clearDisplays().clearElements()


I dont like how the builder is same builder is being reused. Maybe copy from the built model of customModel?

modmuss50 · 2022-12-20T16:05:50Z

...on-api-v1/src/testmod/java/net/fabricmc/fabric/test/datagen/DataGeneratorTestEntrypoint.java

+					.setGuiLight(ItemModelBuilder.GuiLight.SIDE);
+			itemModelGenerator.build(ITEM_WITH_CUSTOM_MODEL_1, customModel);
+
+			customModel.clearOverrides().setGuiLight(null)


Again here, reusing the builder is a little odd imo? Doesnt quite follow the builder design pattern too well. Nither does the clearOverrides() like methods either 🤔

modmuss50 · 2022-12-20T16:07:15Z

fabric-data-generation-api-v1/src/main/resources/fabric.mod.json

+    "loom:injected_interfaces": {
+      "net/minecraft/class_4910": ["net/fabricmc/fabric/api/datagen/v1/model/FabricBlockStateModelGenerator"],
+      "net/minecraft/class_4915": ["net/fabricmc/fabric/api/datagen/v1/model/FabricItemModelGenerator"],
+      "net/minecraft/class_4942": ["net/fabricmc/fabric/impl/datagen/FabricModel"]


Dont use interface injection for impl.

Now that I think about it I'm not sure if this should even be an impl interface given its use in the API-provided model builders. Given the intent to also allow ModelBuilder to be extended for custom models I may also have to reconsider how to go about this to actually allow for any custom properties that other mods may choose to incorporate.

fabric-data-generation-api-v1/src/main/java/net/fabricmc/fabric/impl/datagen/FabricModel.java

…c/api/datagen/v1/FabricModel.java Co-authored-by: BasiqueEvangelist <[email protected]>

(new props now get added to the prior-generated model for whatever reason)

(fixes properties being added to the wrong models)

simplification step 1

This reverts commit 2e61ffc.

This reverts commit f731f11.

(also rebase)

BasiqueEvangelist reviewed Dec 12, 2022

View reviewed changes

fabric-data-generation-api-v1/src/main/java/net/fabricmc/fabric/api/datagen/v1/FabricModel.java Outdated Show resolved Hide resolved

Technici4n reviewed Dec 13, 2022

View reviewed changes

62832 changed the title ~~Extensions to datagen API~~ Extensions to datagen API for model generation Dec 13, 2022

62832 mentioned this pull request Dec 14, 2022

Extra data providers for remaining vanilla asset types #2759

Draft

6 tasks

Technici4n added the in progress This issue has PR(s) open to resolve it (or a PR that is WIP) label Dec 16, 2022

62832 marked this pull request as ready for review December 16, 2022 17:23

modmuss50 reviewed Dec 20, 2022

View reviewed changes

...1/src/main/java/net/fabricmc/fabric/api/datagen/v1/model/FabricBlockStateModelGenerator.java Show resolved Hide resolved

modmuss50 requested changes Dec 20, 2022

View reviewed changes

Technici4n mentioned this pull request Jan 5, 2023

Fix #2552: Add way to use resource conditions for block loot table datagen #2823

Merged

62832 and others added 20 commits January 11, 2023 21:38

Initial test - add setters for simple model props

d830c28

Add builder classes for remaining model properties

07eeb8d

Add injector methods for builders

8256a73

Update fabric-data-generation-api-v1/src/main/java/net/fabricmc/fabri…

ed1b9c8

…c/api/datagen/v1/FabricModel.java Co-authored-by: BasiqueEvangelist <[email protected]>

Clear extra properties after outputting each model

8335208

Move to clearer subpackage

3cc8c03

Address misc comments

6906cbb

Begin work on custom item model generator methods

a5c6de8

Add static model creation methods (TBD)

f9a7ffb

Add more item model generation methods (TBD)

65ff4b2

Begin work on custom block model generator methods

0f4afea

Add GUI light item generator methods

de0d36f

First attempt at fixing premature prop clearing

5ef02ee

(new props now get added to the prior-generated model for whatever reason)

Adapt DisplayBuilder to use Position as ctor param

f944f1e

Wrap up item model methods

683cc27

Switch to dedicated builder classes for models

47b7053

(fixes properties being added to the wrong models)

Some extra methods

ff0536c

Allow ModelBuilder to provide respective texture maps as well

d3e765e

simplification step 1

Allow registering a model directly from a builder

4fb5514

Revert "Allow registering a model directly from a builder"

77ef53a

This reverts commit 2e61ffc.

62832 added 9 commits January 11, 2023 21:41

Revert "Allow ModelBuilder to provide respective texture maps as well"

861aba8

This reverts commit f731f11.

take two

5efbf29

First blockstate method attempt

c38f2b0

Allow removing single textures and displays from builder

351520e

Add removal methods for some property builders

6a23494

yes, they're _builders_

74082d0

Add more test cases

5ccac52

Smaller amendments

a5087a5

Fix incorrectly built "faces" definition

80de375

(also rebase)

62832 force-pushed the datagen-extensions branch from 2fa05ad to 80de375 Compare January 11, 2023 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensions to datagen API for model generation #2754

Extensions to datagen API for model generation #2754

62832 commented Dec 12, 2022

Technici4n left a comment

62832 commented Dec 16, 2022

modmuss50 left a comment

modmuss50 Dec 20, 2022

62832 Dec 20, 2022

modmuss50 Dec 20, 2022

modmuss50 Dec 20, 2022

modmuss50 Dec 20, 2022

62832 Dec 20, 2022

		blockStateModelGenerator.buildWithSingletonState(BLOCK_WITH_CUSTOM_MODEL_1, customModel);

		customModel.clearDisplays().clearElements()

Extensions to datagen API for model generation #2754

Are you sure you want to change the base?

Extensions to datagen API for model generation #2754

Conversation

62832 commented Dec 12, 2022

Technici4n left a comment

Choose a reason for hiding this comment

62832 commented Dec 16, 2022

modmuss50 left a comment

Choose a reason for hiding this comment

modmuss50 Dec 20, 2022

Choose a reason for hiding this comment

62832 Dec 20, 2022

Choose a reason for hiding this comment

modmuss50 Dec 20, 2022

Choose a reason for hiding this comment

modmuss50 Dec 20, 2022

Choose a reason for hiding this comment

modmuss50 Dec 20, 2022

Choose a reason for hiding this comment

62832 Dec 20, 2022

Choose a reason for hiding this comment