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.

Sign up for GitHub

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

Jump to bottom

[1.21.3] RegisterRenderStateModifiersEvent and EntityRenderState extends AttachmentHolder #1650

Open
wants to merge 12 commits into
base: 1.21.x
Choose a base branch
from
Open

[1.21.3] RegisterRenderStateModifiersEvent and EntityRenderState extends AttachmentHolder #1650

wants to merge 12 commits into from

Conversation

dhyces
Copy link
Contributor

@dhyces dhyces commented Oct 31, 2024 •
edited
Loading

Closes #1638

Consider this an RFC atm.

This PR targets extensibility of EntityRenderState due to how inextensible the new render state system is. To do this, we will expand the class with AttachmentHolder to allow users to query attachment data in rendering. The attachments are copied to ensure the values are not mutated between the render state update and the user's custom rendering. The other system added on top allows for adding client-sided elements to render states, used when the data stored is not related to attachments. To accomplish this, a new event is added RegisterRenderStateModifiersEvent. The event currently has only one method, registerEntityModifier, which accepts the targeted renderer class and a modifier function, though should be extended with another method registerItemModifier in 1.21.4 for ItemStackRenderStates. The EntityRenderStateModifier accepts the entity instance and the render state, after calling extractRenderState. Users would then modify the render state by calling IEntityRenderState#setRenderData with a statically initialized ContextKey and the data they wish to store. Every modifier that applies to the renderer is run, including modifiers that target superclasses. These collections are cached to be as efficient as possible.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

@XFactHD XFactHD added enhancement New (or improvement to existing) feature or request rendering Related to rendering 1.21.3 Targeted at Minecraft 1.21.3 labels Oct 31, 2024
@Technici4n
Copy link
Member

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
Copy link
Contributor Author

dhyces commented Nov 1, 2024

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
Copy link
Member

An idea might be to use a context map 🤔

@neoforged-automation
Copy link

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

@dhyces
Draft for event
fab1b31
@dhyces
Example 1
3139c92
@dhyces
Example 2
3c5e760
@dhyces
Cherry-pick changes from example 2 branch
99600ad
@dhyces
Apply Tslat's feedback
f98e132 
1. Moved callsite to `createRenderState`. This differs from feedback, though I believe is a better location, seeing as the method is final and will apply modifiers if users call the method.
2. Some classes have been renamed to better reflect behavior
3. Use ContextKey instead of custom class
4. Retain each modifier separately so that subclasses may be considered
5. Use lazily evaluated map for storing all modifiers that apply to a given renderer
@dhyces
Rename to RegisterRenderStateModifiersEvent
a8ef1cf
@dhyces dhyces changed the title [1.21.3] UpdateRenderStateEvent and EntityRenderState extends AttachmentHolder [1.21.3] RegisterRenderStateModifiersEvent and EntityRenderState extends AttachmentHolder Nov 11, 2024
@dhyces dhyces marked this pull request as ready for review November 14, 2024 23:22
@dhyces dhyces requested a review from XFactHD November 14, 2024 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XFactHD XFactHD Awaiting requested review from XFactHD

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
1.21.3 Targeted at Minecraft 1.21.3 enhancement New (or improvement to existing) feature or request rendering Related to rendering
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Entity's Data Attachments visible through EntityRenderState
3 participants
@dhyces @Technici4n @XFactHD