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

actoranimation.cpp faster getbonebyname #3099

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

bosvensson1
Copy link
Contributor

While reviewing uses of the slow linear search FindByNameVisitor during my last PR, I stumbled upon some uses that we can refactor into a faster logarithmic search using a map already built. Somehow most of these lines are never triggered in my game so I will need an advise if we are using this code anymore and if my changes are OK.

@psi29a
Copy link
Member

psi29a commented Sep 9, 2021

@unelsson gave his blessing here as well. 👍🏼

@psi29a psi29a merged commit c284d0c into OpenMW:master Sep 9, 2021
psi29a added a commit that referenced this pull request Sep 12, 2021
@psi29a
Copy link
Member

psi29a commented Sep 12, 2021

Sorry @bosvensson1 but it seems that this caused an issue for weapon/shield sheathing so I had to revert.

Looks like it isn't used for what we would expect to use it for based on its name (finding bones in the actor's skeleton), instead, it seems to be used to find nodes in attached parts like weapon meshes.

This isn't obvious from the code, only our docs tells you where the nodes are supposed to be.

@akortunov do you have any insight here? Should we perhaps change the name of the function or at least comment about what this does in code?

@bosvensson1
Copy link
Contributor Author

Reviewing the docs of weapon sheathing it seems I made a mistake and we still have to use FindByNameVisitor in most cases. We could still make the searches more efficient by searching only the subgraph where we expect the node to be. Unfortunately the docs do not mention where Bip01 AttachShield is supposed to be. I have moved on to more worthwhile performance improvements in the meantime. Akortunov is welcome to take over the suggestions from this thread if deemed worthwhile.

@akortunov
Copy link
Collaborator

Unfortunately the docs do not mention where Bip01 AttachShield is supposed to be.

It is supposed to be a part of actor scene node.

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.

4 participants