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

add posing registry #9461

Draft
wants to merge 8 commits into
base: 1.20.x
Choose a base branch
from

Conversation

agnor99
Copy link
Contributor

@agnor99 agnor99 commented Apr 22, 2023

This PR adds a new hook to rendering LivingEntities.
The problem with the existing Events (RenderLivingEvent.Pre&RenderPlayerEvent.Pre) is that modifications to the model are impossible, as the rotations and translations are set after the events have been fired.
The Poses are registered client-side and sorted similar to Tiers and the TierSortingRegistry, which allows much finer control over the ordering of poses and which take precedence over others. The hooks allow default transforms to happen, but are also able to cancel all unnecessary posestack and model transformations.
Some poseStack transformations aren't stopped if a pose requests it, but only those that rotate the player in the correct direction and scale it.

I've also moved all of the init calls in the Minecraft class to ForgeHooksClient as this was mentioned multiple times in the discord (example).

@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.19 Feature This request implements a new feature. New Event This request implements a new event. labels Apr 22, 2023
@autoforge autoforge bot requested a review from a team April 22, 2023 23:01
@marchermans
Copy link
Contributor

I am not 100% on this approach.

I understand that we need sorting and the likes so this feels like a natural progression, however I think this section is rather performance heavy, and I am not sure that a straight up iterate over all poses for every living entity is a great idea.

I would like to see some performance numbers on this.

@agnor99
Copy link
Contributor Author

agnor99 commented Apr 24, 2023

I did a test on the performance impact for each rendercall on my machine (intel i5-7500 3,4GHz) and used 50 slightly modified versions of each of the 2 poses in the example mod. Times were measured over 10k rendercalls
Changes to the example mod poses:

  • No cancelling of other rotations so that all 100 poses and the vanilla posing is done.
  • 50 of the instances did an enchantment test (checking if curse of vanishing is present) instead of the leather armor test

Impact:
Average of around 3.3 microseconds per entity, which results to around 1.3ms for 400 livingentities per frame

Impact with no poses registered:
Average of around 0.16 microseconds per entity, which results to around 0.06ms for 400 livingentities per frame

Impact with 100 no-op poses:
A no op pose isn't active, therefore only the performance impact of the feature itself with 100 poses to iterate over are measured.
Average of around 0.66 microseconds per entity, which results to around 0.26ms for 400 livingentities per frame.

Conclusion:
The overhead of this addition are not that big and are largely dependent on the implementation of the pose and how they are implemented

@agnor99
Copy link
Contributor Author

agnor99 commented Apr 24, 2023

what do you not like about this approach?
was it just the potential performance impact or with the system itself and the api?
if so: what can I improve?

@ChampionAsh5357
Copy link
Contributor

So, the few issues I see with it are the following:

  • There is no handling for when vanilla poses should be applied. It is always assumed to be before any modded poses when enabled.
  • There is no good way to handle partial transformations of models such that animations on other parts of the body are unaffected while only applying to the specific circumstance.

Also, I don't see a reason why this wouldn't be bundled with EntityRenderersEvent$AddLayers instead of having its own custom events. As models are rebuilt on client reload, I believe the same should be done for poses as they are part of the model itself.

@agnor99
Copy link
Contributor Author

agnor99 commented Apr 25, 2023

The reason why Vanilla always goes first is because vanilla seems to set all positions and rotations to a value based on certain variables on the entity and would override all changes to the model done by a pose.

There is no good way to handle partial transformations of models such that animations on other parts of the body are unaffected while only applying to the specific circumstance.

I don't understand what you mean by this tbh, can you rephrase this?

@ChampionAsh5357
Copy link
Contributor

I don't understand what you mean by this tbh, can you rephrase this

Basically, if a pose only affected an arm while another pose only affected a leg and they both said stop using other poses. They should be considered exclusive if the poses have no effect on each other and should be executed. This is most likely the case with custom animations for a given state while an additional flag on the entity changes how the model should be rendered.

@agnor99
Copy link
Contributor Author

agnor99 commented Apr 25, 2023

The I'm the last pose, don't use anything else was intended for a pose that changes all parts like skiing where it wouldn't make much sense to try to change other positions. Changes to only a single part shouldn't mark themselves as the last pose (except they want to make the player animation stop completly)

@sciwhiz12 sciwhiz12 added the Rendering This request deals with rendering. These must be treated with care, and tend to progress slowly. label Apr 25, 2023
@agnor99
Copy link
Contributor Author

agnor99 commented May 12, 2023

Should I undo the move of the Initialzation to ForgeHooksClient, with the event now being fired after the AddLayers Event and not during the running of the Minecraft constructor?

@autoforge autoforge bot added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label May 14, 2023
@autoforge
Copy link

autoforge bot commented May 14, 2023

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

@autoforge autoforge bot removed the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label May 16, 2023
@ChampionAsh5357
Copy link
Contributor

Sorry for the late response.

The I'm the last pose, don't use anything else was intended for a pose that changes all parts like skiing where it wouldn't make much sense to try to change other positions. Changes to only a single part shouldn't mark themselves as the last pose (except they want to make the player animation stop completly)

Sure, but are you supposed to register a pose per part if you want to have it more cleanly interop with other transformations? Let me describe a use case. We have the following three pose transformations from different mods:

  • A pose that transforms the arm based upon the action the entity is performing with an item.
  • A pose that transforms the arm by default into a new format, preventing everything else from being transformed.
  • A pose that transforms the legs by default into a new format, preventing everything else from being transformed.

Mods are not expected to know about other mods by default. As such, how can you guarantee the order of which the poses are applied? Additionally, there are two parts which both specify that nothing should go after it, but they can be combined as the arm and leg are separate parts of the body. As such, you could apply both of them as they operate on different parts of the body.

Essentially, the system provided for registration requires the mods to know context about other mods to work effectively. It's not like there are vanilla poses already that the system can have as some sort of bounds. The only way I can see this working is if you can define some sort of layering system of what type of transformation a pose is so that a mod does not need to be aware of every other mod which uses this system.

As a side note, there needs to be more documentation of everything, and I would rather have the pose registry methods be part of the AddLayers event since a completely separate event is unnecessary here.

@ChampionAsh5357 ChampionAsh5357 added the Work In Progress This request has lots of changes that need attention. label May 17, 2023
@agnor99
Copy link
Contributor Author

agnor99 commented May 17, 2023

  • A pose that transforms the arm based upon the action the entity is performing with an item.
  • A pose that transforms the arm by default into a new format, preventing everything else from being transformed.
  • A pose that transforms the legs by default into a new format, preventing everything else from being transformed.

Pose 2 and 3 are using this system wrong. Those poses would cancel all vanilla animations (like walking, crouching, swimming [...]), which is not intended for just changing the arms/legs and a misuse of the api. If now Pose 2 is not cancelling all other poses, this system would be able to handle it, by using the sorting feature to apply the first pose after the second, conditionally overriding the arm transformation. More documentation on IPose would probably help with the understanding of this feature.

Keeping track of specific transformation types is imo not beneficial, as there would be too many types. humanoid_legs, humanoid_arms, humanoid_left_arm, shulker_top_shell, bee_wings and many more. Some of them inherit other transformation types (like humoid_arms inherits humanoid_left_arm). Preventing other poses for fully custom animations makes sense, but not for changed arm positions, as those should just set the intended values after vanilla has set default ones.

@ChampionAsh5357
Copy link
Contributor

More documentation on IPose would probably help with the understanding of this feature.

Yeah, I'm still only partially understanding how this is supposed to be used and applied, so apologies.

Keeping track of specific transformation types is imo not beneficial, as there would be too many types. humanoid_legs, humanoid_arms, humanoid_left_arm, shulker_top_shell, bee_wings and many more.

That's fair. However, I still believe there needs to be some concept of layering that a mod can know 'before this' or 'after that' rather than only for their own items without necessarily needing context from other mods. Maybe something like a tool action if you want abstract representations for types or the event priority system such that there is some context a mod can glean from a pose.

@agnor99
Copy link
Contributor Author

agnor99 commented May 17, 2023

I designed based on the two major reasons I could think of how modders would like to modify the posing of the model, slight tweaks and complete changes.
Slight tweaks would contain all things that doesn't involve the whole player model like waving with the arm or changing the default arm positions.
Complete changes would contain all things that manipulate all model parts for full body animations which could be used for new forms of transport or idling animations where the player sits down after a bit of time of inactivity.
These complete changes would change anything that vanilla does and all changes done by other poses, therefore they can return true for IPose#shouldOtherPosesActivate() to mark themselves as the last pose applied and not breaking this full body animation without knowing all other poses to defined the pose as running after all other poses.
These slight tweak poses could be passed through, so they wouldn't return true for IPose#shouldOtherPosesActivate(). The layering could be done by using the befores and afters during the pose registration. This would require knowledge of the other poses existance. Logging the poses ResourceLocation in sorted order and if they are blocking could be enough to fix mod incompatabilities over time I think.
The problem with a second priority system on top of the sorted registry, is conflicting data.
Assume pose1 is registered and asks to be rendered after pose2, but pose2 has a priority conflicting with that. I don't see a good way, or a really good reason to add an additional sorting layer on top of the sorting registry

@ChampionAsh5357
Copy link
Contributor

I designed based on the two major reasons I could think of how modders would like to modify the posing of the model, slight tweaks and complete changes.

Ok, so why not split it into those two sections: have a place where slight tweaks are rendered first and then complete changes are rendered second. That's enough context such that mods don't necessarily need to be aware of each other if they aren't specified.

This would require knowledge of the other poses existance. Logging the poses ResourceLocation in sorted order and if they are blocking could be enough to fix mod incompatabilities over time I think.
The problem with a second priority system on top of the sorted registry, is conflicting data. Assume pose1 is registered and asks to be rendered after pose2, but pose2 has a priority conflicting with that. I don't see a good way, or a really good reason to add an additional sorting layer on top of the sorting registry

Sure, but then this still doesn't address the issue of what is the default ordering. There has to be an answer rather than a mod must know information about someone else's mod. The topological sort would essentially be a list for all intensive purposes if the mods have no concept of each other, which means that poses that stops activation of all subsequent poses could be run first by coincidence (don't say anything about event priorities, that is not a valid solution as it delegates responsibility to the event system rather than your pose registry). Unless you can address some sort of context or default that doesn't rely on a mod having knowledge of another, I don't see this PR as being complete. I can also step back if you believe me to be in the wrong, though I don't believe you'll get any further with another triage member.

@agnor99
Copy link
Contributor Author

agnor99 commented May 17, 2023

I can definetly see the reason to split it into two parts.
Would it be enough sorting control, to have two lists that people could add to and people can sort themselves using the event priority?

@XFactHD
Copy link
Contributor

XFactHD commented May 17, 2023

You can't rely on event priority for this because the event fires on the mod bus, which essentially ignores priorities since priorities are per event bus and every mod has its own event bus.

@ChampionAsh5357
Copy link
Contributor

Would it be enough sorting control, to have two lists that people could add to and people can sort themselves using the event priority?

If the intended use case is that there are only two types: tweaks and replacements, sure. It would probably be better to put this for discussion on forgecord though to verify that everything can be boiled down into these two and that there's no major concern when dealing with multiple replacements from different mods.

You can't rely on event priority for this because the event fires on the mod bus, which essentially ignores priorities since priorities are per event bus and every mod has its own event bus.

Right...

@agnor99 agnor99 changed the base branch from 1.19.x to 1.20.x June 23, 2023 21:16
# Conflicts:
#    patches/minecraft/net/minecraft/client/Minecraft.java.patch
@agnor99
Copy link
Contributor Author

agnor99 commented Jun 23, 2023

I've updated the PR to target 1.20.x, changed the sorting to sort by an enum instead of poses sort against each other and improved the javadoc. Is there more I can do for this PR?

@autoforge autoforge bot added the Needs Rebase This PR requires a rebase to implement upstream changes (usually to fix merge conflicts). label Jun 27, 2023
@autoforge
Copy link

autoforge bot commented Jun 27, 2023

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

@LexManos
Copy link
Member

Alright with everything going on, I am going to convert this PR into a draft. It seems to be a bit niche for Forge. It also effects the rendering side which is something I'll be working on getting up to speed on. I'm sorry if you don't like this outcome but for the time being its not gunna be going in. If you decide to keep this open when I get around to the rendering side of the project then I can reevaluate it. But don't expect it for 1.20.1's lifecycle.

@LexManos LexManos marked this pull request as draft July 14, 2023 21:14
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.19 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. Rendering This request deals with rendering. These must be treated with care, and tend to progress slowly. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. Work In Progress This request has lots of changes that need attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants