-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Data driven models and animations #145
base: 1.20
Are you sure you want to change the base?
Conversation
@FoundationGames this PR uses code from JsonEM. On discord, you said it was fine for me to use the code. I am asking here for a more visible record. I still need to update the licenses for said files. |
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 approve the merging of any code within this pull request that has been borrowed from JsonEM.
...odels/src/main/java/org/quiltmc/qsl/rendering/entity_models/mixin/ModelPartDataAccessor.java
Outdated
Show resolved
Hide resolved
...els/src/main/java/org/quiltmc/qsl/rendering/entity_models/impl/DynamicEntityModelLoader.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.
Lots of super little nitpicks but it looks great!
...ntity_models/src/main/java/org/quiltmc/qsl/rendering/entity_models/api/AnimationManager.java
Outdated
Show resolved
Hide resolved
...ntity_models/src/main/java/org/quiltmc/qsl/rendering/entity_models/api/AnimationManager.java
Outdated
Show resolved
Hide resolved
...ntity_models/src/main/java/org/quiltmc/qsl/rendering/entity_models/api/AnimationManager.java
Outdated
Show resolved
Hide resolved
...endering/entity_models/src/main/java/org/quiltmc/qsl/rendering/entity_models/api/Codecs.java
Outdated
Show resolved
Hide resolved
...endering/entity_models/src/main/java/org/quiltmc/qsl/rendering/entity_models/api/Codecs.java
Outdated
Show resolved
Hide resolved
...els/src/main/java/org/quiltmc/qsl/rendering/entity_models/impl/DynamicEntityModelLoader.java
Outdated
Show resolved
Hide resolved
...els/src/main/java/org/quiltmc/qsl/rendering/entity_models/impl/DynamicEntityModelLoader.java
Outdated
Show resolved
Hide resolved
...els/src/main/java/org/quiltmc/qsl/rendering/entity_models/impl/DynamicEntityModelLoader.java
Outdated
Show resolved
Hide resolved
...ls/src/main/java/org/quiltmc/qsl/rendering/entity_models/impl/LazyTypeUnboundedMapCodec.java
Show resolved
Hide resolved
...in/java/org/quiltmc/qsl/rendering/entity_models/mixin/EntityRendererFactoryContextMixin.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.
So far, so great! I only have style nitpicks for now, and I'd appreciate if you did a pass through the code with Checkstyle;
Considering the majorness of this PR, I'll want to experiment with it on practice later
...ty_models/src/main/java/org/quiltmc/qsl/rendering/entity_models/api/HasAnimationManager.java
Outdated
Show resolved
Hide resolved
library/rendering/entity_models/src/main/resources/quilt_entity_models.mixins.json
Outdated
Show resolved
Hide resolved
...ng/entity_models/src/main/java/org/quiltmc/qsl/rendering/entity_models/api/package-info.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Ennui Langeweile <[email protected]>
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.
CI's broken due to missing headers; Fix CI, and I'll re-review again :P
…braries into data-driven-models
Note. This pr will break geckolib, so maybe we should figure something out before killing that mod |
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 PR now makes more sense to me!
...main/java/org/quiltmc/qsl/rendering/entity_models/api/animation/AnimationInterpolations.java
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.
You'll need to copy the icon again! I had ran oxipng through everything :p
|
||
@Invoker("<init>") | ||
static ModelCuboidData create(@Nullable String string, float f, float g, float h, float i, float j, float k, float l, float m, Dilation dilation, boolean bl, float n, float o, Set<Direction> directions) { | ||
throw new AssertionError("Mixin injection failed."); |
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.
throw new AssertionError("Mixin injection failed."); | |
throw new IllegalStateException("Mixin injection failed."); |
Set<Direction> directions(); | ||
|
||
@Invoker("<init>") | ||
static ModelCuboidData create(@Nullable String string, float f, float g, float h, float i, float j, float k, float l, float m, Dilation dilation, boolean bl, float n, float o, Set<Direction> directions) { |
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 do wonder if this method has arg names now
public record AnimationType(Codec<Animation> codec) { | ||
} |
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.
public record AnimationType(Codec<Animation> codec) { | |
} | |
public record AnimationType(Codec<Animation> codec) {} |
TODO:
Don't use regex for model layer parsing