Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.21.3] Expose vanilla model generators for modded usages #1696

Draft
wants to merge 23 commits into
base: 1.21.x
Choose a base branch
from

Conversation

ApexModder
Copy link
Contributor

@ApexModder ApexModder commented Nov 22, 2024

This PR allows modders to make use of the vanilla model generators and all the helpers/utilities that come with them.
This is done by adding methods which modders must override in each of the main data providers, failing to override these methods will result in vanilla models being generated.

Various ResourceLocation.withDefaultNamespace calls have also been patched to now call ResourceLocation.parse, this is to support passing in paths with modded namespaces.

With 1.21.4 and all of its model changes, this is intended as a stepping point towards using the vanilla generators. Rather than having to patch and potentially rewrite ours to support all these changes.

TODO

  • Add ExistingFileHelper support, validate referenced textures and parent models exist
  • Expand ModelBuilder to pass its output to ItemModelOutput, allowing modders to build custom models
  • Support our RenderType and face data extensions when using template models
    • Unsure if mutating face data is possible using model templates
  • Generate texture animation mcmeta files?
  • Validate that added ATs are still needed after recent changes.

Original conversation can be found in NeoCord

@ApexModder ApexModder added the 1.21.3 Targeted at Minecraft 1.21.3 label Nov 22, 2024
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Nov 22, 2024

  • Publish PR to GitHub Packages

Last commit published: 7f75f6c00501a40a6b4ce62e6b8aab83884d98f6.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1696' // https://github.com/neoforged/NeoForge/pull/1696
        url 'https://prmaven.neoforged.net/NeoForge/pr1696'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1696.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1696
cd NeoForge-pr1696
curl -L https://prmaven.neoforged.net/NeoForge/pr1696/net/neoforged/neoforge/21.3.62-beta-pr-1696-pr-vanilla-data-gen/mdk-pr1696.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

@ApexModder ApexModder added 1.21.4 Targeted at Minecraft 1.21.4 and removed 1.21.3 Targeted at Minecraft 1.21.3 labels Nov 23, 2024
JsonObject jsonobject = new JsonObject();
this.model.ifPresent(p_176461_ -> jsonobject.addProperty("parent", p_176461_.toString()));
- this.model.ifPresent(p_176461_ -> jsonobject.addProperty("parent", p_176461_.toString()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of modifying the two lambdas, I would do the two checks at the top of the method:

if (fileHelper != null) {
    this.model.ifPresent(parent -> Preconditions.checkState(fileHelper.exists(parent, ModelProvider.MODEL), "Model at %s does not exist", parent));
    p_266912_.forEach((slot, name) -> Preconditions.checkState(fileHelper.exists(name, ModelProvider.TEXTURE), "Texture %s does not exist in any known resource pack", name));
}

// ExistingFileHelper is nullable for backwards compat with vanilla data gen
// should never/rarely ever be null in modded data gen
var fileHelper = Objects.requireNonNull(self().fileHelper, "Looking up models requires a nonnull ExistingFileHelper");
return new ModelFile.ExistingModelFile(modelPath.withPath(IBlockModelGenerators::appendBlockFolder), fileHelper);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return new ModelFile.ExistingModelFile(modelPath.withPath(IBlockModelGenerators::appendBlockFolder), fileHelper);
ModelFile model = new ModelFile.ExistingModelFile(modelPath.withPath(IBlockModelGenerators::appendBlockFolder), fileHelper);
model.assertExistence();
return model;

// ExistingFileHelper is nullable for backwards compat with vanilla data gen
// should never/rarely ever be null in modded data gen
var fileHelper = Objects.requireNonNull(self().fileHelper, "Looking up models requires a nonnull ExistingFileHelper");
return new ModelFile.ExistingModelFile(modelPath.withPath(IItemModelGenerators::appendItemFolder), fileHelper);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return new ModelFile.ExistingModelFile(modelPath.withPath(IItemModelGenerators::appendItemFolder), fileHelper);
ModelFile model = new ModelFile.ExistingModelFile(modelPath.withPath(IItemModelGenerators::appendItemFolder), fileHelper);
model.assertExistence();
return model;

// ExistingFileHelper is nullable for backwards compat with vanilla data gen
// should never/rarely ever be null in modded data gen
var fileHelper = Objects.requireNonNull(self().fileHelper, "Looking up models requires a nonnull ExistingFileHelper");
return new ModelFile.ExistingModelFile(modelPath, fileHelper);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return new ModelFile.ExistingModelFile(modelPath, fileHelper);
ModelFile model = new ModelFile.ExistingModelFile(modelPath, fileHelper);
model.assertExistence();
return model;

default ResourceLocation create(Block block, TextureMapping textures, BiConsumer<ResourceLocation, Supplier<JsonElement>> modelOutput, @Nullable ExistingFileHelper fileHelper) {
return create(ModelLocationUtils.getModelLocation(block, self().suffix.orElse("")), textures, modelOutput, fileHelper);
default ModelTemplate withRenderType(String renderType) {
return withRenderType(ResourceLocation.withDefaultNamespace(renderType));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return withRenderType(ResourceLocation.withDefaultNamespace(renderType));
return withRenderType(ResourceLocation.parse(renderType));

return json;
}

@Override
public JsonObject createBaseTemplate(ResourceLocation modelPath, Map<TextureSlot, ResourceLocation> textureMap) {
return createBaseTemplate(modelPath, textureMap, null);
}

public static JsonObject toJson(ItemTransform transform) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toJson() methods need distinct names

+ private final String modId;
+ @org.jetbrains.annotations.Nullable private final net.neoforged.neoforge.common.data.ExistingFileHelper fileHelper;
+ public final String modId;
+ @org.jetbrains.annotations.Nullable public final net.neoforged.neoforge.common.data.ExistingFileHelper fileHelper;

- public ModelProvider(PackOutput p_252226_) {
+ public ModelProvider(PackOutput p_252226_, String modId, @org.jetbrains.annotations.Nullable net.neoforged.neoforge.common.data.ExistingFileHelper fileHelper) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch around the order of the constructors to make the patch a bit more readable

new ItemModelGenerators.TrimModelData("amethyst", 1.0F, Map.of())
);
public final BiConsumer<ResourceLocation, Supplier<JsonElement>> output;
+ @org.jetbrains.annotations.Nullable public final net.neoforged.neoforge.common.data.ExistingFileHelper fileHelper;
+ public final String modId;

- public ItemModelGenerators(BiConsumer<ResourceLocation, Supplier<JsonElement>> p_125082_) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch around the order of the constructors to make the patch a bit more readable

@@ -64,23 +64,25 @@
+ }
+
+ @org.jetbrains.annotations.Nullable public final net.neoforged.neoforge.common.data.ExistingFileHelper fileHelper;
+ public final String modId;
+
public BlockModelGenerators(
- Consumer<BlockStateGenerator> p_124481_, BiConsumer<ResourceLocation, Supplier<JsonElement>> p_124482_, Consumer<Item> p_124483_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch around the order of the constructors to make the patch a bit more readable


+ // Neo: Allow passing model builders into block state variants
+ // Not to be used in conjunction with 'MODEL'
+ public static final VariantProperty<net.neoforged.neoforge.client.model.generators.BlockModelBuilder> MODEL_BUILDER = new VariantProperty<>("model", model -> new JsonPrimitive(model.getLocation().toString()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is particularly useful. The variant stuff is already pretty verbose, so it's unlikely someone would nest an entire model builder chain in that code, and pulling the location from a builder to pass to the vanilla VariantProperties.MODEL is trivial

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.4 Targeted at Minecraft 1.21.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants