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

Data driven models and animations #145

Open
wants to merge 19 commits into
base: 1.20
Choose a base branch
from

Conversation

OroArmor
Copy link
Member

@OroArmor OroArmor commented Jun 25, 2022

TODO:

  • Documentation
  • Mixin clean ups
  • Don't use regex for model layer parsing

@OroArmor OroArmor added new: module A pull request which adds a new module new: library A pull request which adds a new library. t: new api This adds a new API. s: not working This pull request has been tested and confirmed as not working for now. labels Jun 25, 2022
@OroArmor OroArmor added s: wip This pull request is being worked on. and removed s: not working This pull request has been tested and confirmed as not working for now. labels Jul 3, 2022
@OroArmor OroArmor marked this pull request as ready for review July 30, 2022 08:19
@OroArmor OroArmor requested a review from a team as a code owner July 30, 2022 08:19
@OroArmor OroArmor added the test label Jul 30, 2022
@OroArmor
Copy link
Member Author

OroArmor commented Aug 2, 2022

@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.

Copy link
Contributor

@FoundationGames FoundationGames left a 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.

@TheGlitch76 TheGlitch76 marked this pull request as draft August 22, 2022 22:04
@TheGlitch76 TheGlitch76 removed the test label Aug 28, 2022
@OroArmor OroArmor marked this pull request as ready for review September 23, 2022 18:55
Copy link
Contributor

@Platymemo Platymemo left a 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!

@OroArmor OroArmor added s: tested This pull request has been tested and confirmed as working. and removed s: wip This pull request is being worked on. labels Sep 28, 2022
Copy link
Contributor

@EnnuiL EnnuiL left a 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

@OroArmor OroArmor changed the base branch from 1.19 to 1.19.3 December 17, 2022 21:59
Copy link
Contributor

@EnnuiL EnnuiL left a 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

@OroArmor
Copy link
Member Author

OroArmor commented Jan 3, 2023

Note. This pr will break geckolib, so maybe we should figure something out before killing that mod

@OroArmor OroArmor changed the base branch from 1.19.3 to 1.20 June 8, 2023 23:25
Copy link
Contributor

@EnnuiL EnnuiL left a 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!

Copy link
Contributor

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.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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) {
Copy link
Contributor

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

Comment on lines +28 to +29
public record AnimationType(Codec<Animation> codec) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public record AnimationType(Codec<Animation> codec) {
}
public record AnimationType(Codec<Animation> codec) {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new: library A pull request which adds a new library. new: module A pull request which adds a new module s: tested This pull request has been tested and confirmed as working. t: new api This adds a new API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants