-
-
Notifications
You must be signed in to change notification settings - Fork 183
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.4] Expose vanilla model generators for modded usages #1725
base: 1.21.x
Are you sure you want to change the base?
Conversation
Last commit published: c351c859d1d2faa560c3fd4a85830b69b4cb8cb4. PR PublishingThe artifacts published by this PR:
Repository DeclarationIn order to use the artifacts published by the PR, add the following repository to your buildscript: repositories {
maven {
name 'Maven for PR #1725' // https://github.com/neoforged/NeoForge/pull/1725
url 'https://prmaven.neoforged.net/NeoForge/pr1725'
content {
includeModule('net.neoforged', 'neoforge')
includeModule('net.neoforged', 'testframework')
}
}
} MDK installationIn order to setup a MDK using the latest PR version, run the following commands in a terminal. mkdir NeoForge-pr1725
cd NeoForge-pr1725
curl -L https://prmaven.neoforged.net/NeoForge/pr1725/net/neoforged/neoforge/21.4.93-beta-pr-1725-pr-vanilla-datagen-21.4/mdk-pr1725.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. |
408ec8a
to
41e1c12
Compare
@ApexModder, this pull request has conflicts, please resolve them for this PR to move forward. |
41e1c12
to
3d04267
Compare
@ApexModder, this PR introduces breaking changes. Compatibility checks
|
This PR should now be ready for view but important note Meaning that the following would change to something like so |
@ApexModder, this pull request has conflicts, please resolve them for this PR to move forward. |
4e6095e
to
b910484
Compare
@ApexModder, this pull request has conflicts, please resolve them for this PR to move forward. |
dced55d
to
0e4ac40
Compare
@ApexModder, this pull request has conflicts, please resolve them for this PR to move forward. |
943368e
to
916a5e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more comments, primarily about documentation.
When these are adressed, this PR is good to go from my end. The approval and merge has to be done by someone else as this PR adds code I wrote.
|
||
void copy(Item p_387316_, Item p_387995_); | ||
+ | ||
+ default void register(Item item, net.minecraft.client.renderer.item.ClientItem clientItem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a comment mentioning that Neo pulls this up into the interface to allow mods to control the entire ClientItem
for an item and not just the ItemModel.Unbaked
it uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide JDoc on what this method does or is supposed to do after it now has been pulled up.
We can not expect tools like Parchment to provide this to modders so we should be libraral here with the explanation even if that increases the patch size.
patches/net/minecraft/client/data/models/ModelProvider.java.patch
Outdated
Show resolved
Hide resolved
patches/net/minecraft/client/data/models/ModelProvider.java.patch
Outdated
Show resolved
Hide resolved
...main/java/net/neoforged/neoforge/client/model/generators/template/RootTransformsBuilder.java
Outdated
Show resolved
Hide resolved
...main/java/net/neoforged/neoforge/client/model/generators/template/RootTransformsBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/client/model/generators/template/RotationBuilder.java
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/client/model/generators/template/RotationBuilder.java
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/client/model/generators/template/TransformVecBuilder.java
Outdated
Show resolved
Hide resolved
patches/net/minecraft/client/data/models/ModelProvider.java.patch
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/client/model/generators/template/TransformVecBuilder.java
Outdated
Show resolved
Hide resolved
patches/net/minecraft/client/data/models/ItemModelOutput.java.patch
Outdated
Show resolved
Hide resolved
PR should be ready for review and merge, only unresolved comment should be xfacts one about |
|
||
void copy(Item p_387316_, Item p_387995_); | ||
+ | ||
+ default void register(Item item, net.minecraft.client.renderer.item.ClientItem clientItem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide JDoc on what this method does or is supposed to do after it now has been pulled up.
We can not expect tools like Parchment to provide this to modders so we should be libraral here with the explanation even if that increases the patch size.
import net.minecraft.world.level.block.Block; | ||
import net.minecraft.world.level.block.state.properties.BlockStateProperties; | ||
|
||
public interface IBlockModelGeneratorsExtension { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extension is not strictly necessary.
It is possible to create a custom BlockFamily that is incomplete and use it to perform these generations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove it.
src/main/java/net/neoforged/neoforge/common/data/ExistingFileHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor things excluding what Orion said. I also removed my comments about the ExistingFileHelper
if we are planning to strip it out.
|
||
void copy(Item p_387316_, Item p_387995_); | ||
+ | ||
+ // Neo: Pulled up from 'ModelProvider.ItemInfoCollector' to give modders full control over the 'ClientItem' instead of just the 'ItemModel.Unbacked' in '#accept' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ // Neo: Pulled up from 'ModelProvider.ItemInfoCollector' to give modders full control over the 'ClientItem' instead of just the 'ItemModel.Unbacked' in '#accept' | |
+ // Neo: Pulled up from 'ModelProvider.ItemInfoCollector' to give modders full control over the 'ClientItem' instead of just the 'ItemModel.Unbaked' in '#accept' |
default TexturedModel updateTemplate(UnaryOperator<ModelTemplate> modifier) { | ||
return new TexturedModel(self().getMapping(), modifier.apply(self().getTemplate())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this needs some documentation to explain why updateTemplate
makes sense, since the additional data used by the custom loaders is set on the template itself rather than the textures slots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this could be moved to an extension method on ItemModelGenerators
rather than being outright removed as it still is a neoforge-added item model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason custom model loaders have builders is because they aren't reversible and therefore not serializable. The dynamic fluid container model is a custom ItemModel
now though and the associated ItemModel.Unbaked
can just be instantiated and passed to ItemModelOutput#register()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. I just figured it would probably be easier to have a convenience method to create them, similar to all the methods in the model generator.
protected static void serializeNestedTemplate(ModelTemplate template, TextureMapping textures, Consumer<JsonElement> consumer) { | ||
template.create(DUMMY, textures, (id, jsonSup) -> consumer.accept(jsonSup.get())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation? I don't know what this would be for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method was unused and thus has been removed, was told its better practice to now reference external files rather than inlining models like we used to, which this was used for in the past.
Updated version of #1696 for
1.21.4
.While the vanilla providers have been exposed for modded usage, and can be used for more simpler model generation. This is still highly WIP and requires more patches to support our model extensions and generating a model from scratch.