-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Optimize mesh animation: Hardware skinning #16721
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
base: master
Are you sure you want to change the base?
Conversation
as an abstract class for vertex and index buffers to inherit from.
fixme seems to cause a segfault in opengl driver? (if that isn't an earlier regression...)
|
performance improvement on my machine: great work! 👍 |
|
Seems to work. Some indication in the profiler would be nice - number of vertex attributes uploaded (or whatever makes sense). |
irr/src/COpenGLDriver.cpp
Outdated
| } else { | ||
| assert(b->IndexBuffer); | ||
| if (b->ChangedID != b->IndexBuffer->getChangedID() || !b->vbo_ID) { | ||
| } else if (auto *ib = dynamic_cast<const scene::IIndexBuffer *>(b->Buffer)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so happy about dynamic_cast in hot code paths. is this good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe not. it does seem like this might have some unwanted cost if there are very many small buffers.
i've replaced it with a virtual function call to get the buffer type and a switch.
|
I had a this applied for the past days... I have not observed anything weird or unexpected. |
|
What's in the way of this? |
|
@appgurueu did you check if this works with dynamic shadows? |
2af1782 to
d43f657
Compare
|
Good point. I tested it. Shadows do break with this, as the shader used for rendering won't do the skinning, so shadows are cast for the static poses. It's relatively easy to do a simple fix by also applying the skinning logic in the appropriate shadow shader. That's what I've done for now. There is a good chance that accessing disabled vertex attributes and running a simple check on them is cheap enough that the overhead doesn't matter, but maybe I should make sure that scene nodes that don't need skinning get a simple shader with zero overhead? |
|
What is the max number of bone influences per vertex with this PR? |
|
The limit is at 4 per vertex, introduced by #16655 |
This moves skinning (transformation of vertices by bone transforms) from the CPU ("software") to the GPU ("hardware") on the OpenGL 3 driver in order to improve performance. This is done by moving weights to vertex buffers and uploading them as vec4 vertex attributes (weights + joint IDs). Matrices are uploaded as a UBO.
There is some refactoring here that could maybe be split off into a separate PR.
The gains are unfortunately limited a bit by the computation of the joint transforms that still takes place on the CPU. This can be optimized further (and is already optimized a bit here, e.g. by eliminating some useless conversions and optimizing the Transform -> matrix conversion), but that should probably be done in a follow-up PR. (In particular, this can in principle be parallelized well, and we might not want to spread our bones on the heap. Matrix multiplication can maybe also be optimized further using SIMD.)
Closes #9218.
To do
This PR is Ready for Review.
How to test
Use the
/spider_armycommand (included in the devtest gltf mod) to spawn 10³ spiders in the air. Observe them and note the FPS. You should see something like a ~2x increase vs master on the OGL 3 driver.Also make sure that the fallback to SW skinning works as expected: Try the
opengldriver, editgetMaxJointTransforms()toreturn 0in the OGL 3 driver.