[1.21.3] RegisterRenderStateModifiersEvent for appending custom data to render state objects

[dhyces]

Closes #1638

This PR targets extensibility of EntityRenderState, MapRenderState, and MapDecorationRenderState due to how inextensible the new render state system is. The system added on top allows for adding client-sided elements to render states. To accomplish this, a new event is added RegisterRenderStateModifiersEvent. The event supplies several methods for registering modifiers for the various types of render state objects. Should be extended with another method registerItemModifier in 1.21.4 for ItemStackRenderStates. Users would then add to the render state by calling IRenderStateExtension#setRenderData with a statically initialized ContextKey and the data they wish to store. Vanilla data may also be modified (at the user's discretion). For entity render states, every modifier that applies to the given renderer class is run, including modifiers that target superclasses, and are cached to be as efficient as possible.

[neoforged-pr-publishing]

• Publish PR to GitHub Packages

Last commit published: d400b27441cfe82f25d2a0d5fc21c986361b6233.

PR Publishing

The artifacts published by this PR:

• 📦 net.neoforged:testframework:21.3.88-beta-pr-1650-feat-render-state-event
• 📦 net.neoforged:neoforge:21.3.88-beta-pr-1650-feat-render-state-event

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1650' // https://github.com/neoforged/NeoForge/pull/1650
        url 'https://prmaven.neoforged.net/NeoForge/pr1650'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1650.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1650
cd NeoForge-pr1650
curl -L https://prmaven.neoforged.net/NeoForge/pr1650/net/neoforged/neoforge/21.3.88-beta-pr-1650-feat-render-state-event/mdk-pr1650.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.

[Technici4n]

I think that having:

• The attachment holder (which is weird if it's mutable I think).
• The ad-hoc key system to add extensions.
• The extension/merger system.
  is 3 ways of achieving essentially the same thing. That's too much. I think there should only be one way.

Also, are there thread-safety or similar issues that would force us to make the data immutable? Or is it fine to directly reference the entity or something that it contains?

[dhyces]

I think that having:

• The attachment holder (which is weird if it's mutable I think).
• The ad-hoc key system to add extensions.
• The extension/merger system.
  is 3 ways of achieving essentially the same thing. That's too much. I think there should only be one way.

Also, are there thread-safety or similar issues that would force us to make the data immutable? Or is it fine to directly reference the entity or something that it contains?

I mentioned in the description that the key system and extension system were two different examples for how it could work, only one would be implemented. I somewhat agree that the attachments would be enough (and yes, while it's mutable, it's a copy and wouldn't affect the client entity), though there may be some aspects that simply would be better off not using attachments and should use something else to extend the RenderState. Tslat mentioned in neocord that generic extension outside of attachments would be necessary.

[Technici4n]

An idea might be to use a context map 🤔

[neoforged-automation]

@dhyces, this pull request has conflicts, please resolve them for this PR to move forward.

[XFactHD]

The overall approach seems sane to me. One thing I'm thinking about though is whether it would make sense to make these extensions a bit more generic from the start by using two base classes for them which the vanilla ones extend from instead of patching the entire machinery into each of the respective vanilla "base" render state class (in this case EntityRenderState). What I'm imagining is a BaseRenderState or AbstractRenderState that holds the generic extension part and a AttachmentAwareRenderState which extends the former and implements IAttachmentHolder. The attachment holder implementation would be immutable which may also allow using an unmodifiable view of the entity's attachments map instead of copying it. The vanilla EntityRenderState (and the respective class for BEs if BERs get the same treatment) would then extend AttachmentAwareRenderState while MapRenderState, MapDecorationRenderState and ItemStackRenderState would extend BaseRenderState. The state modifier registration and retrieval would of course still have to be per "base type" (i.e. entity, map, map decoration, BE, itemstack).

[dhyces]

The overall approach seems sane to me. One thing I'm thinking about though is whether it would make sense to make these extensions a bit more generic from the start by using two base classes for them which the vanilla ones extend from instead of patching the entire machinery into each of the respective vanilla "base" render state class (in this case EntityRenderState). What I'm imagining is a BaseRenderState or AbstractRenderState that holds the generic extension part and a AttachmentAwareRenderState which extends the former and implements IAttachmentHolder. The attachment holder implementation would be immutable which may also allow using an unmodifiable view of the entity's attachments map instead of copying it. The vanilla EntityRenderState (and the respective class for BEs if BERs get the same treatment) would then extend AttachmentAwareRenderState while MapRenderState, MapDecorationRenderState and ItemStackRenderState would extend BaseRenderState. The state modifier registration and retrieval would of course still have to be per "base type" (i.e. entity, map, map decoration, BE, itemstack).

I like this idea a lot. I haven't yet looked into the Map... render states, so I'm not sure how that could be useful. Could you provide an example? I also have a concern with the attachment stuff, since we provide the arbitrary render data already, should we just tell users that if they want to access some data from their synced attachments, they can use a modifier to copy it over themselves? That way everything's not copied every time regardless of whether it's used or not.

[XFactHD]

While testing, I found that registering modifiers for generic renderers leads to really ugly, unchecked casts in the registration. For example, registering a modifier for any LivingEntityRenderer ends up with this:

event.registerEntityModifier(
    (Class<? extends EntityRenderer<LivingEntity, LivingEntityRenderState>>) LivingEntityRenderer.class,
    (entity, renderState) -> {}
);

since you can't specify generics in a class literal. I'm not sure there is anything we can realistically do about this though without just entirely dropping type safety.

[XFactHD]

@neoforged/bots run applyAllFormatting

[neoforged-automation]

Running Gradle tasks applyAllFormatting...

[neoforged-automation]

Workflow failed: https://github.com/neoforged/PRActionRunner/actions/runs/11944387911

[lukebemish]

I'd make use of type tokens for checking compatibility in that check -- the current logic has some substantial holes in that and a type token super/subclass comparison lets you avoid all those. If not, please make sure there's no wacky edge cases here in any case -- that's a subtle bug caused by some particular mod interaction waiting to happen. If you're not making use of compile time type safety, the runtime type safety needs to be quite strong so it's guaranteed to fire in dev if something could go wrong.