-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Added CPU blendshapes for GLES2 #48480
Conversation
The basic structure looks good. It is missing 2 things:
I will leave @lawnjelly to discuss how this can be made to work on GLES3 and GLES2 as the software skeleton transform does. I am not convinced software blendshapes are necessary for GLES3 (the only need as far as I can tell is to work around a particular driver bug). |
It's difficult because clayjohn may be right about it only being used in GLES2. It's difficult to make that call at this stage, but it strikes me it would be a lot of extra work to change down the line, which would be unlikely to happen, so we end up having to commit to particular path early on. I can't immediately see any major downsides to doing it in the client code like the software skinning, compared to in the rasterizer itself. Some thoughts:
I'll try and have a proper look tomorrow as it is getting late here, and work out how the existing blendshapes work, I haven't looked or tried these (so maybe as clayjohn is more familiar he may be right with his intuitions). |
Wonderful thoughts, as always. My responses below:
The issue with putting the software implementation into client code is that users have to force enable software fallbacks if they think that any of their users will experience the driver bug. We need a solution that just works (TM).
With this PR, blendshapes and skinning have the same relationship that they do in GLES3. So if something is wrong when combing blendshapes and skinning then it needs to be addressed for both. If it does turn out to be an issue, then certainly we should kill two birds with one stone and implement software blendshapes client side.
This sounds very interesting, but I don't know enough about the software skinning to know if it will be valuable. Overall, I think we should implement blend shapes in the rasterizer in GLES2 and then evaluate whether it makes sense to add a client-side software blend shapes implementation that replaces it (the same way that the current software skinning replaces the hardware skinning). I know it would feel redundant, but this PR won't be very much code even when finished) and it makes sense to me to have proper blend shape support in the rasterizer. |
Ah I get you, sorry I got my wires crossed. You are meaning make a hardware implementation for GLES2 (I was for some reason thinking you meant a software CPU solution in the rasterizer), yes it didn't occur to me as I'm not familiar with blend shapes. So I'm in agreement that a hardware GPU version would be worth trying. 👍 |
@clayjohn I have now added blending to normals, colors and uv maps and made it work on both normalized and relative modes. I wasn't sure how exactly weight/bones blend so i excluded them, but it seems to work anyways on my animated skeleton/blendshape test project. Let me know if there's anything else to fix. |
No, I do mean have a software implementation in the rasterizer and a software implementation in the client the way we have with skeletons. In GLES2, if hardware doesn't support float textures we fall back to a software implementation. This is different from the software skinning implemented client side. |
Alright, I've added tangent and weight blending now. |
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.
Looks good to me. I tested with https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/MorphStressTest and the other morph files in the repo.
I am a little concerned about the extra vertex pressure because this forces every model using blendshapes to de-compress vertex data before passing it to the GPU. This results in slightly more expensive CPU-GPU transfers and more vertex pressure when issuing the draw command. That being said, I'm not sure storing a GPU buffer per mesh, configured for each mesh's combination of compression formats is worthwhile.
I'm happy to merge this once CI passes and @lawnjelly has had one more chance to review it and express his concerns.
Fantastic work to get this done in such a short time for a new contributor. I'm happy with if clayjohn has tested, it looks well done from a quick look. Only question, as I'm not familiar with blend shapes in godot, can they be combined with skinning, and if so, are the results the same when using this with software skinning or using this with hardware skinning? The classic example would be using blendshapes for a face / head for talking, used in combination with skinning for the bones. But a simpler version would do to test it. BTW, even if it doesn't interact correctly, as is this is still a valid addition of functionality as many won't be using that combination, so I'm happy to merge in either case. 👍 |
Yes, i was also thinking about this situation especially on weaker platforms such as android. An idea I had was to only recalculate the blend shapes when the mesh has changed (blend values, blend data, mesh data, etc) with like a dirty flag or something. At least for now it would be fairly easy to find your bottleneck by simply hiding objects. |
@lawnjelly it should work fine with the rasterizer based skinning (both hardware and software) and it should work with client-side software skinning (as this changes the vertex attributes at the last moment before rendering) unfortunately, I don't have a model to test with that has both skeleton and blendshapes. |
Thanks! |
@clayjohn you said:
I'm currently optimizing this implementation since performance was pretty bad in a real world case. I was able to implement a CPU/GPU buffer for each surface, and it works much better - framerate now only drops when the blend shape is changed. I justify the added memory usage by only creating it for meshes that actually has blendshapes. The previous code did upload shape data to the gpu without ever using it, anyways. I'm curious what your thoughts were regarding what you wrote, what should I look out for? |
@parulina I was concerned about memory usage. If there is enough speed improvement then it is probably worthwhile. |
Is it difficult to add this to gles3? |
@parulina Are you planning on making a PR with the changes? |
I totally forgot! I can clean up/rebase and submit it for tomorrow. |
That would be much appreciated. Thank you :) |
Was CPU Blendshapes added to GLES3? I wasn't sure. |
@fire No, this PR was specific to the GLES2 backend. |
Basic software blendshapes for GLES2 renderer. So far, I have tested this on windows.
I'm not too experienced with pull requests so let me know if I should change anything
Fixes #36612
Bugsquad edit: This closes #48427.