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

Simplify skinning to always expect VEC4 joint/weight accessors. #8846

Merged
merged 3 commits into from
May 13, 2020

Conversation

emackey
Copy link
Contributor

@emackey emackey commented May 13, 2020

A few weeks ago I was implementing skinning in a desktop product, and looked to Cesium's implementation for some inspiration. This is a story of what I unearthed.

In glTF 2.0, the JOINTS_0 and WEIGHTS_0 accessors are specified to be vertex attributes of type VEC4 only. But in glTF 1.0, this was left unspecified. Normal software that writes this stuff always wrote VEC4 data, but our intrepid intern at the time took the loose specification to mean that any type of accessor could appear there.

As a result, a complex block of JavaScript writes skinning GLSL vertex shader code for any conceivable layout of joint+weight data, including the possibility of using a per-vertex matrix of references to joint matricies. There is no known sample or test data that does this, and indeed this code was not covered by unit tests for any case other than the VEC4 case alone.

In this PR, I've replaced all the crazy skinning-shader-writing code with a copy of the correct output for VEC4 which should always have been the only allowed case. As a precaution I put in a sanity check to disable skinning on a mesh in case an invalid format is ever found in this field.

I tested this with the four old sample glTF 1.0 files, to confirm they all use VEC4, and yes, they do and they work correctly. I also briefly tested with a glTF 2.0 file, but I'm not as concerned there since the spec is a little tighter.

I expect no change in behavior, this is just a simplification (and removal of untested, presumed-dead code for dealing with matrices of matrices).

@cesium-concierge
Copy link

Thanks for the pull request @emackey!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@@ -4388,7 +4388,7 @@ function applySkins(model) {
computedJointMatrices[m]
);
if (defined(bindShapeMatrix)) {
// Optimization for when bind shape matrix is the identity.
// NOTE: bindShapeMatrix is glTF 1.0 only, removed in glTF 2.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC CesiumGS/gltf-pipeline#403 - ultimately bindShapeMatrix should be removed in upgradeVersion.

@lilleyse
Copy link
Contributor

I'm seeing a crash for materials-common models like https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/1.0/BrainStem/glTF-MaterialsCommon.

error

Though to be fair, master doesn't animate it correctly.

brainstem

@emackey
Copy link
Contributor Author

emackey commented May 13, 2020

Thanks, forgot to actually test materialsCommon. The crash in this PR was an old variable skinMat that got renamed to skinMatrix during my hasty copy-and-paste. The stiff joint behavior in master is due to an old set of semantics JOINT and WEIGHT which were renamed years ago to JOINTS_0 and WEIGHTS_0 for glTF 2.0.

Both should be fixed now.

@emackey
Copy link
Contributor Author

emackey commented May 13, 2020

Wait wait wait... just spotted something odd.

In the completed vertex shader, the log depth system is not told about skinning deformations. Uh?

void czm_depth_main() {
    mat4 skinMatrix =
        a_weight.x * u_jointMatrix[int(a_joint.x)] +
        a_weight.y * u_jointMatrix[int(a_joint.y)] +
        a_weight.z * u_jointMatrix[int(a_joint.z)] +
        a_weight.w * u_jointMatrix[int(a_joint.w)];
  vec4 pos = u_modelViewMatrix * skinMatrix * vec4(a_position,1.0);
  v_positionEC = pos.xyz;
  gl_Position = u_projectionMatrix * pos;
  v_normal = u_normalMatrix * mat3(skinMatrix) * a_normal;
}

void main() 
{ 
    czm_depth_main(); 
    czm_vertexLogDepth(u_projectionMatrix * u_modelViewMatrix * vec4(a_position.xyz, 1.0)); 
       // Wait... where did skinMatrix go??
} 

@emackey
Copy link
Contributor Author

emackey commented May 13, 2020

OK I'm out of my element with the logDepth stuff, and it's unrelated to this PR.

But in master, I know that skinned models don't have proper depth buffering. And in this vertex shader output, I can see that the czm_vertexLogDepth thingie is clearly being told about the mesh's rest (bind pose) position, without any influence from skinning, so that's a huge red flag.

But the code that generates that code is... interesting. Can't quite follow the logic there yet.

Can anyone more familiar with logDepth advise? Specifically:

  • Why is the argument to czm_vertexLogDepth re-calculating a value that was already calculated in the czm_depth_main() function?
  • What is the best way to make sure that all of czm_depth_main's calculations are properly reflected in the log depth system?

@lilleyse
Copy link
Contributor

@emackey was that a workaround to fix #6447? Based on the discussion there the workaround might not be needed, and model can go back to not messing with log depth.

@emackey
Copy link
Contributor Author

emackey commented May 13, 2020

Yes, looks like if the workaround is pulled, some glTF 2.0 models are fixed (like BearOnBalloons from #6416), but models with non-normalized bone weights are then broken (such as the glTF 1.0 version of CesiumMan).

I'd rather see the glTF 2.0 models work than the 1.0 models, so it may be time to pull that workaround. That could be a separate PR though from this.

@lilleyse
Copy link
Contributor

@emackey agreed, can't really justify keeping that workaround in just for some skinned 1.0 models.

@lilleyse
Copy link
Contributor

All looks good here.

@lilleyse lilleyse merged commit d07271e into master May 13, 2020
@lilleyse lilleyse deleted the gltf-skinning-what-the- branch May 13, 2020 20:19
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.

3 participants