bates64 / papermario-dx

Base for Paper Mario romhacks
80 stars 25 forks source link

AnimatorNode cache for ModelAnimator #92

Closed JCog closed 2 weeks ago

JCog commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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