OGRECave / ogre

scene-oriented, flexible 3D engine (C++, Python, C#, Java)
https://ogrecave.github.io/ogre/
MIT License
3.91k stars 963 forks source link

Always provide bones matrices if hw anim enabled. #3043

Closed erikogenvik closed 6 months ago

erikogenvik commented 6 months ago

This fixes an issue where no bones matrices would be generated if there was no active animation. This would result in incorrect vertex calculations, since the shaders expect bone matrices independently of whether there's an active animation or not.

I ran into this issue after upgrading Ogre and enabling World Space Bones. We use our own shaders and I adjusted them and everything ran nicely, until I had a situation with a mesh that didn't have any active animations. The vertices would get messed up. This change fixed that, but it's not completely obvious to me how this worked previously (with local bones) but not now (with world space bones), so I'm a bit cautious about this fix. It needs some more eyes.

paroj commented 6 months ago

this does not seem to be right.

you are basically disabling this code path: https://github.com/OGRECave/ogre/blob/1cc00496287583aa0e85aaf25eee2ad8b91dfc74/OgreMain/src/OgreSubEntity.cpp#L200

however it should be correct, it applies the parent transform to all bones, ignoring any bone matrices - i.e. the mesh is in its binding pose.

upgrading Ogre and enabling World Space Bones.

note that object space bones is the new and faster way. see: https://www.ogre3d.org/2022/10/29/ogre-13-5-released#object-space-bones

The vertices would get messed up.

can you add a screenshot of this and provide a minimal reproducer example? Also, does this happen only with your shaders or with the RTSS too?

erikogenvik commented 6 months ago

Sorry, I got the methods mixed up. This is when I enable object space bones, not the other way around.

So, the way I understand it is that when object space bones are enabled, the first entry in world_*array/bone*_array is the world transform, with the the rest being filled in. The issue with the code in question is that the length of this array would differ depending on whether there was an animation enabled or not. Here's a screenshot of how it looks with one of our "pig" meshes, which has a skeleton and uses hw skinning, but where there's at the moment no active animation. image

You can also recreate this by commenting out this line https://github.com/OGRECave/ogre/blob/master/Samples/Simple/include/SkeletalAnimation.h#L261 and then running the "Skeletal Animation" in the Sample Browser. This will cause a divide by zero assert when run in debug mode and probably a crash when run in release. I haven't looked more closely into exactly why, but it seems like the DQ conversion tries to decompose a zero matrix which doesn't work.

So this issue is solved by making sure that the world transform matrix always return the same sized array independently of whether there's an active animation or not.

Another solution for this specific issue is to alter the code in line 200 to be

std::fill_n(xform, indexMap.size() + (MeshManager::getBonesUseObjectSpace() ? 1 : 0), mParentEntity->_getParentNodeFullTransform());

but I thought that the code was more easy to read if the addition of the world transform at index 0 happened at start.

The other issue I ran into was that even with that fix (making sure the array length was consistent) my shaders would work incorrectly. The result was that when an hw skinned mesh didn't have any active animations it wouldn't be rendered at all. At least what I could see. I didn't investigate further exactly why since that issue also was solved by switching from checking "_isSkeletonAnimated" to instead just checking "hasSkeleton()". This is where I'm more hesitant since that issue might be caused by my shaders. But switching that check made it work for me.

paroj commented 6 months ago

Sorry, I got the methods mixed up.

ah.. it makes more sense now. This should be the fix: #3044

can you verify?

erikogenvik commented 6 months ago

Nice, this fixes the issue.

paroj commented 6 months ago

replaced by #3044