fabric-model-events-api

[Sollace]

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:

• Mod A is a backpack mod. It wants to render a backpack onto the player's back whenever their item is equipped.
• Mod B is "Mine Little Pony". It replaces the player's entire model with that of a four-legged creature.

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:

ModelPartCallbacks.get(PartTreePath.of(EntityModelPartNames.HEAD)).register(EntityType.PLAYER,
        (player, partView, matrices, vertexConsumer, tickDelta, light, overlay, r, g, b, a) -> {
     partView.part().pivotX = -5;
     partView.part().render(matrices, vertexConsumer, light, overlay, r, g, b, a);
     partView.part().pivotX = 4;
});

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:

ModelPartCallbacks.get(ModelPartCallbacks.MatchingStrategy.STARTS_WITH, PartTreePath.of(EntityModelPartNames.HEAD)).register(EntityType.PLAYER, 
        (player, partView, matrices, vertexConsumer, tickDelta, light, overlay, r, g, b, a) -> {
    // ...
});

[Syst3ms]

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.

[Syst3ms]

This is unnecessary, loom will add this annotation to all impl packages automatically.

[Syst3ms]

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.

[Sollace]

Yes, that's the idea. It's meant to run last.

[Syst3ms]

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.

[Sollace]

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.

[Syst3ms]

What happened with the indentation there? Again, run checkstyle, it will definitely catch that and then some.

[Sollace]

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.

[Syst3ms]

Should this be thread-safe?

[Sollace]

I'm not concerned about thread safety since all rendering is done on the same thread.

[Juuxel]

The module name should be fabric-model-events-v1.

fabric/CONTRIBUTING.md

Lines 289 to 291 in a462da6

	- Module names should usually be suffixed by `-api`.
	- Modules whose primary purpose is not interaction with their API do not need this suffix. For example, `fabric-transitive-access-wideners-v1` or `fabric-convention-tags-v1`.
	- Event modules should have the `-events` suffix instead.

[Sollace]

The module name should be fabric-model-events-v1.

fabric/CONTRIBUTING.md

Lines 289 to 291 in a462da6

	- Module names should usually be suffixed by `-api`.
	- Modules whose primary purpose is not interaction with their API do not need this suffix. For example, `fabric-transitive-access-wideners-v1` or `fabric-convention-tags-v1`.
	- Event modules should have the `-events` suffix instead.

The thing you quoted appears to disagree...?

[Juuxel]

Event modules should have the -events suffix instead.

This seems to be an event module?

[Sollace]

Event modules should have the -events suffix instead.

This seems to be an event module?

Hm okay

[apple502j]

Oh no, another module. Could we just add this to Model Loader or Rendering APIs?

[apple502j]

Ideally these should be written in JUnit.

[Sollace]

I dunno, man. I copied this from an existing module.

[apple502j]

The usual Java name is "startsWith".

[Sollace]

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)

[LlamaLad7]

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 of on_render.

[Sollace]

Something I'd like some feedback on:

1. Thread safety in ModelRenderContext.java.
   Since this is entirely connected to rendering, I would presume this is always being accessed from the same thread. It's pretty easy for me to fix this, I just want to know if this is this a real concern I should address?

2. Preconditions
   In a few places I am using Objects and Preconditions for doing sanity checks. Is there a preferred alternative I should be using here instead?

3. Event parameters
   I've included what I think is a reasonable set of parameters, basically the same as what you'd need to specify when calling ModelPart#render. I recognise there might be some use in providing the VertexConsumers in use as well. Is there something else worth considering for this?

   Options are either I can add it as a parameter to the event, change the existing parameter (not really a fan as they already have a lot) or add additional getters to PartView (as that is intended to be a convenient expansion point to the api as it can serve the role as a Event object.

4. Cancelling
   It's technically possible to allow mods to cancel part rendering by adding a boolean return. I thought about doing that, but then again the handler has all the power to change the model part instance they're given.
   (Although hiding it may be a special case for providing specific support for cancelling as simply setting ModelPart#hidden = true would also prevent future invokations of the event)

5. Traversal
   I'm on the fence about keeping that. It's technically possible to manually iterate the data views, the traverser can just do it a little more efficiently since it has access to api internals. I'm not a fan of constructing them the way they are being done, however.

   The traverser does have the small advantage that it has baked-in handling for all the matrix stack transforming and rotating needed to render stuff where the part is during traversal (which is how I produced the screenshots in the OP)

6. No lists
   The DataCollection class is not a list, nor did I ever have any intention for them to be used like one (even though they have random access and iteration). They're there as a way to get the first and last element as an optional, with values computed and cached internally upon demand so we're not heavily impacting render performance.

   I also anticipate that someone could optimize them further (and I likely will revise it myself during the course of this PR)

[Juuxel]

In a few places I am using Objects and Preconditions for doing sanity checks. Is there a preferred alternative I should be using here instead?

Both of those are good options, the contributing guidelines mention Objects directly too and Preconditions is also widely used.