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

AnimatorNode cache for ModelAnimator #92

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

JCog
Copy link
Contributor

@JCog JCog commented Sep 9, 2024

get_animator_child_with_id() searches for child AnimatorNodes recursively using a depth-first search, which in practice is pretty slow. It gets called numerous times for many (most?) animated entities and is the source of a ton of lag. This simply implements a cache of these nodes by id for ModelAnimator, improving performance significantly.

As a side note, do we care too much about labeling the offsets of struct members? I understand why they're there for decomp, but updating them here is tedious and I'm not sure if there's a point.

@z64a
Copy link
Collaborator

z64a commented Sep 9, 2024

Looks good to me. I'm fine with removing offset annotations for changed structs, not sure how others feel about it. They can be useful when stepping through instructions hunting down crashes, but they're not as crucial now that we don't have any functions left to match in dx. I'm also in favor of removing padding fields (like unk_08F here) from structs we've modified.

@bates64
Copy link
Owner

bates64 commented Sep 9, 2024

I disagree about removing offset comments and padding fields, unless the struct was changed to make them incorrect. Offsets are often helpful when debugging, and removing them from structs will introduce git conflicts if is renamed upstream. Padding can be helpful to know about to avoid growing structs that be susceptible to shifting issues - these are uncommon now but I think still they exist. For new structs or heavily changed ones I think it's fine to get rid of these things though. I personally wouldn't bother updating offsets

@bates64 bates64 merged commit c8bb67e into bates64:main Sep 9, 2024
3 of 4 checks passed
@JCog JCog deleted the entity-node-cache branch September 9, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants