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
base: 1.20.x
Are you sure you want to change the base?
add posing registry #9461
Conversation
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. |
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
Impact: Impact with no poses registered: Impact with 100 no-op poses: Conclusion: |
what do you not like about this approach? |
So, the few issues I see with it are the following:
Also, I don't see a reason why this wouldn't be bundled with |
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.
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. |
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) |
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? |
@agnor99, this pull request has conflicts, please resolve them for this PR to move forward. |
3ed1c5a
to
ebf94e9
Compare
Sorry for the late response.
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:
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 |
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. |
Yeah, I'm still only partially understanding how this is supposed to be used and applied, so apologies.
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. |
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.
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. |
I can definetly see the reason to split it into two parts. |
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. |
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.
Right... |
e72bb16
to
745125b
Compare
# Conflicts: # patches/minecraft/net/minecraft/client/Minecraft.java.patch
745125b
to
08a0924
Compare
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? |
@agnor99, this pull request has conflicts, please resolve them for this PR to move forward. |
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. |
|
cb455e6
to
2d5ba40
Compare
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).