-
Notifications
You must be signed in to change notification settings - Fork 399
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
fabric-model-events-api #3523
base: 1.20.4
Are you sure you want to change the base?
fabric-model-events-api #3523
Conversation
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'm not familiar with rendering code, so I can't judge the core API design, so this is just about general code structure and stuff I caught. This direly needs checkstyle.
|
||
import net.fabricmc.fabric.api.client.modelevents.v1.data.DataCollection; | ||
|
||
@ApiStatus.Internal |
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 is unnecessary, loom will add this annotation to all impl packages automatically.
...pi-v1/src/client/java/net/fabricmc/fabric/api/client/modelevents/v1/data/DataCollection.java
Show resolved
Hide resolved
import net.minecraft.util.math.Direction; | ||
|
||
@ApiStatus.Internal | ||
@Mixin(value = ModelPart.Cuboid.class, priority = Integer.MAX_VALUE) |
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.
Mixin priority is generally useless for FAPI. Besides, a lower priority value means the mixin gets triggered earlier, so you gave this the smallest priority possible.
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.
Yes, that's the idea. It's meant to run last.
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.
MAX_VALUE
seems a bit overkill for that. The vast majority of mixins don't use priority at all, so a priority of 1001 could be enough (FAPI uses a priority of 999 in many places to trigger before mods). I'd like a second opinion on this though.
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 lost track of who asked for this - I have changed it to 90000 with a comment explanation. I'm always up to change it further if more people will chime in.
...i-v1/src/client/java/net/fabricmc/fabric/mixin/client/modelevents/ModelPart_CuboidMixin.java
Outdated
Show resolved
Hide resolved
...rc/client/java/net/fabricmc/fabric/mixin/client/modelevents/EntityRenderDispatcherMixin.java
Show resolved
Hide resolved
...ents-api-v1/src/client/java/net/fabricmc/fabric/mixin/client/modelevents/ModelPartMixin.java
Show resolved
Hide resolved
test(errors, ModelEventsTest::checkPathIndexOfForShortPath); | ||
test(errors, ModelEventsTest::checkPathIndexOfForLongPath); | ||
test(errors, ModelEventsTest::checkPathComparisons); | ||
test(errors, ModelEventsTest::registerEntityModelPartListener); |
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.
What happened with the indentation there? Again, run checkstyle, it will definitely catch that and then some.
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 file is a bit weird with spaces and tabs. It's kind of hard to fix inside the IDE since it renders both the same and (confusingly) retains the mix of spaces and tabs when adding new lines.
I'll fix it later.
...pi-v1/src/client/java/net/fabricmc/fabric/api/client/modelevents/v1/data/DataCollection.java
Show resolved
Hide resolved
|
||
@ApiStatus.Internal | ||
public final class ModelRenderContext { | ||
static final Stack<Entity> CURRENT_ENTITY = new ObjectArrayList<>(); |
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.
Should this be thread-safe?
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'm not concerned about thread safety since all rendering is done on the same thread.
The module name should be Lines 289 to 291 in a462da6
|
The thing you quoted appears to disagree...? |
This seems to be an event module? |
Hm okay |
fabric-model-events-api-v1/src/client/resources/assets/fabric-mining-level-api-v1/icon.png
Outdated
Show resolved
Hide resolved
Oh no, another module. Could we just add this to Model Loader or Rendering APIs? |
...api-v1/src/client/java/net/fabricmc/fabric/api/client/modelevents/v1/ModelPartCallbacks.java
Show resolved
Hide resolved
...vents-api-v1/src/client/java/net/fabricmc/fabric/api/client/modelevents/v1/PartTreePath.java
Show resolved
Hide resolved
...nts-api-v1/src/testmod/java/net/fabricmc/fabric/test/client/modelevents/ModelEventsTest.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
static void checkEmptyPartPathCreation() { |
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.
Ideally these should be written in JUnit.
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 dunno, man. I copied this from an existing module.
fabric-model-events-api-v1/src/client/resources/fabric.mod.json
Outdated
Show resolved
Hide resolved
...api-v1/src/client/java/net/fabricmc/fabric/api/client/modelevents/v1/ModelPartCallbacks.java
Outdated
Show resolved
Hide resolved
...vents-api-v1/src/client/java/net/fabricmc/fabric/api/client/modelevents/v1/PartTreePath.java
Show resolved
Hide resolved
/** | ||
* Checks whether the specified part name corresponds to the first element in this path. | ||
*/ | ||
default boolean beginsWith(PartTreePath path) { |
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 usual Java name is "startsWith".
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 kind of want to distenguish this from Strings though. pathA.beginsWith(pathB)
isn't necessarily the same as pathA.toString().startsWith(pathB.toString())
(former is an array-based contents comparison and the latter is per-character which may create false positives)
...ents-api-v1/src/client/java/net/fabricmc/fabric/api/client/modelevents/v1/data/FaceData.java
Show resolved
Hide resolved
...ents-api-v1/src/client/java/net/fabricmc/fabric/api/client/modelevents/v1/data/PartView.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.
General points:
- Never use
MAX_VALUE
as a priority. That just makes it incredibly annoying for mods who want to be after it. I see no reason to use anything over say 2000. - Handler methods are meant to be descriptive of their purposes. Your names just restate what is shown in the annotations already and are not useful in a stacktrace. E.g.
renderFabricPartHooks
instead ofon_render
.
...rc/client/java/net/fabricmc/fabric/mixin/client/modelevents/EntityRenderDispatcherMixin.java
Outdated
Show resolved
Hide resolved
...ents-api-v1/src/client/java/net/fabricmc/fabric/mixin/client/modelevents/ModelPartMixin.java
Outdated
Show resolved
Hide resolved
...i-v1/src/client/java/net/fabricmc/fabric/mixin/client/modelevents/ModelPart_CuboidMixin.java
Outdated
Show resolved
Hide resolved
Co-authored-by: apple502j <33279053+apple502j@users.noreply.github.com>
…/api/client/modelevents/v1/ModelPartCallbacks.java Co-authored-by: apple502j <33279053+apple502j@users.noreply.github.com>
fabric-model-events-api-v1/src/client/resources/fabric-model-events-api-v1.accesswidener
Show resolved
Hide resolved
Something I'd like some feedback on:
|
Both of those are good options, the contributing guidelines mention |
This is a draft!
I'm still working on it, but in the meantime I want to get feedback and ideas on how to improve.
What is this?
The goal of this PR is to introduce API to Fabric that will allow mods to more easily render additions to player and entity models. I wanted to make it possible for a mod to subscribe to receieve events whenever a specific named part of a model is rendered (such as the player's head). They should then recieve as part of the event enough information to render additional accessories (like a hat) onto the player's head using the top face as a point of fixture.
The head is a trivial example, though, so to try for something a bit more complicated, consider this scenario:
If Mod A hardcodes the position and orientation of its backpack, when Mod B is installed, the backpack may appear in the wrong location (clipping into the pony's back). Mod A would have to add specific support for Mod B in order for them both to render in a compatible way, which puts the burden of implenting this onto Mod A's developers who may not be interested in doing this.
This API is intended to resolve this (and potentially several others), in addition to providing better tools for mods to add more complex additions to player models).
If instead, Mod A implemented the rendering for their backpack using this API, they can instead add an event for when the player's torso is rendered, grab the north face, and orient their backpack to that.
Then, so long as the part for the player's torso is oriented correctly by Mod B, the backpack from Mod A will always appear correctly on the differently-shaped model.
Things this API adds
There are three types of events:
The general event
This is fired when a model part is rendered. Doesn't matter when, you get notified of it. The only context you get for this event listener is the part and whatever was given when rendering it.
The entity event
This one is specifically only fired when a model is being rendered for an entity (via EntityRendererDispatcher).
You get notified when the model is being rendered for an entity matching the type you specify, and you receive the entity as part of your context.
The block entity event
Same as the entity event but for block entities
PartView, CubeData, FaceData
The ModelPart being rendered is provided in the form of a PartView which gives you readonly access to pretty much everything you need to know about the ModelPart and its contents.
ModelVisitor
PartViews can be traversed using a visitor pattern. You can create a visitor using ModelVisitor.builder() and specify actions to perform for a Part, a Cube, and a Face. Or you can just query the data using PartView.cubes()
When registering for an event you specify the name/path of the part you want to get events for, and register with one of the three types of event listeners.
Example that would give the player a second head:
You can also specify a
MatchingStrategy
(default is ENDS_WITH) to change the behaviour in how it decides which parts match the path given.This example will fire for any parts contained within the player's head: