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

Guaranteed Execution Order for Render Living Modification #9684

Open
wants to merge 9 commits into
base: 1.20.x
Choose a base branch
from

Conversation

Kovak
Copy link
Contributor

@Kovak Kovak commented Jul 26, 2023

Supersedes #9672, addresses #9118

This PR does a few things:

  1. Replace RenderLiving.Pre and Post with a single event, this accomplishing ensuring that if a pre event is called a post event is called to avoid 2 types of rendering issues:
  • A case where rendering is occurring in the pre event while that even is cancelled by a lower priority handler, resulting in rendering when there should be none
  • A case where drawing state such as the pose stack is manipulated in the pre event with the goal of restoring that state later in the post event, but because of cancellation by a lower priority handler only the pre logic occurred and not the post.
  1. There has been API drift in the underlying Minecraft classes, the render function has a yaw argument being passed into it that we are not preserving at the time "p_115309_" has now been added to the new event as the yaw parameter

  2. Guarantees that the order that post handlers are called is the exact reverse of the order the pre handlers are called. For certain types of render modification this needs to be true. Things like state modification of pose stack need this to guarantee their callers are not operating on another modders state, right now it is possible that Mod A and B could be popping each others pushes instead of unwinding the stack appropriately.

  3. Reduces the number of events dispatched per entity rendering. My previous attempt at solving RenderLivingEvent Posestacks #9118 introduced a third event, increasing overhead. This solution actually reduces the overhead from 2 events to 1 while increasing the guarantees around rendering.

  4. Introduces some examples that confirm that the 2 major problems are fixed, to see how the problems manifest in the current state of forge take a look at rendering examples to demonstrate current issues with pre/post events #9674

@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.20 Feature This request implements a new feature. New Event This request implements a new event. labels Jul 26, 2023
@autoforge autoforge bot requested a review from a team July 26, 2023 19:48
@autoforge autoforge bot added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label Sep 21, 2023
@autoforge
Copy link

autoforge bot commented Sep 21, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Feature This request implements a new feature. Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). New Event This request implements a new event. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant