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

Skinned Mesh attributes #441

Closed
wants to merge 3 commits into from
Closed

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented May 3, 2020

Hey @mosra !

here's some initial draft for Skinning stuff to start discussion.

Cheers,
Jonathan

Open Questions
1.Naming of Attributes

TODOs

  • Add Weights and JointIds to AttributeNames and Generic attributes.
  • MeshData & Testing
  • Documentation?

@mosra mosra marked this pull request as draft May 3, 2020 14:35
@mosra mosra changed the title [WIP] Skinned Mesh attributes Skinned Mesh attributes May 3, 2020
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

And here you have some models with more than 4 weights: https://twitter.com/zeuxcg/status/1194650202696605697

src/Magnum/Shaders/Generic.h Outdated Show resolved Hide resolved
src/Magnum/Shaders/Generic.h Outdated Show resolved Hide resolved
* @see @ref jointIndicesInto(), @ref attributeFormat(),
* @ref isVertexFormatImplementationSpecific()
*/
Containers::Array<Vector4us> jointIndicesAsArray(UnsignedInt id = 0) const;
Copy link
Owner

Choose a reason for hiding this comment

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

I think should always be the largest type (even if it will never be as big):

Suggested change
Containers::Array<Vector4us> jointIndicesAsArray(UnsignedInt id = 0) const;
Containers::Array<Vector4ui> jointIndicesAsArray(UnsignedInt id = 0) const;

src/Magnum/Shaders/Generic.h Outdated Show resolved Hide resolved
src/Magnum/Trade/MeshData.h Outdated Show resolved Hide resolved
src/Magnum/Shaders/Generic.h Outdated Show resolved Hide resolved
src/Magnum/Shaders/Generic.h Outdated Show resolved Hide resolved
@mosra
Copy link
Owner

mosra commented May 8, 2020

What's left for this to be useful is adding those to MeshTools::compile() -- but to test it there, you would need some shader that supports them, and at that point you have implemented skinning completely :)

@Squareys
Copy link
Contributor Author

Squareys commented May 9, 2020

Sure, it looks straight forward enough to do so, and I'll have to eventually anyways, so why not rip off the bandage.

@Squareys
Copy link
Contributor Author

@mosra I started implementing Skinning in Phong to test the additions to compile(), but this would blow up the PR significantly. Could we split Phong shader implementation, compile() and test off into a separate PR?

@mosra
Copy link
Owner

mosra commented May 11, 2020

Yup, good idea -- the Phong implementation also needs tests on its own, so it makes sense to do all that separately.

@Squareys
Copy link
Contributor Author

Split Phong implementation into Squareys:phong-skinning, will create the PR for that once this is merged.
Rebased this and squashed some commits, I think this is ready for review :)

@Squareys Squareys marked this pull request as ready for review May 11, 2020 12:55
* @ref Magnum::Vector4ui "Vector4ui", four joint indices that may affect the vertex.
* Corresponds to @ref Trade::MeshAttribute::JointIds.
*/
typedef GL::Attribute<7, Vector4ui> JointIds;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see the Secondary attribs documented here.

CORRADE_COMPARE(out.str(),
"Trade::MeshData::jointIdsInto(): expected a view with 3 elements but got 2\n");
}

Copy link
Owner

Choose a reason for hiding this comment

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

The attributeNotFound() and implementationSpecificVertexFormatWrongAccess() tests need to be extended to check asserts in the new accessors.

* Corresponds to @ref Shaders::Generic::JointIds.
* @see @ref MeshData::jointIdsAsArray()
*/
JointIds,
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering -- would it make sense to support also one/two/three-component weights and joints? I guess it would be useful for file size savings. Can you check if there are example glTF files with less than 4 weights?

This won't complicate the implementation too much, a similar thing is already done for colors for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The accessor is always a 4-component vector, so in case there is a file like this, it would have 4 but the unused components would be zero weight

Copy link
Owner

Choose a reason for hiding this comment

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

In (unextended) glTF yes, but in general not. I don't see why glTF couldn't support SCALAR, VEC2 and VEC3 also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, if it would make sense to support one/two/three-component weights, would I do that with JointIds1/2/3/4? And I assume the respective changes in MeshData like for color3/4?

Copy link
Owner

Choose a reason for hiding this comment

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

It would be just JointIds always, but allowing also one/two/three-component types instead of just Vectot4ui.

MeshData::jointIdsInto() will then need to zero-fill the unused components like colorsInto() does for three-component colors or positions3DInto() does for two-component positions, yes.

@Squareys Squareys mentioned this pull request May 11, 2020
typedef GL::Attribute<7, Vector4> JointIds;

typedef GL::Attribute<10, Vector4> SecondaryWeights;
typedef GL::Attribute<11, Vector4> SecondaryJointIds;
Copy link
Owner

Choose a reason for hiding this comment

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

Seeing #444, this PR should contain also the generic.glsl changes and corresponding additions to GenericTest::glslMatch()

@mosra
Copy link
Owner

mosra commented Dec 15, 2022

Merged with various changes as 7f94a6f and f447e99. The primary difference is that MeshAttribute::JointIds and Weights is now an array attribute in order to allow a single attribute to have any number of components -- two, four but also for example seven. This allows both for better packing in memory (compared to always having 4 or 8 components) as well as easier processing because with > 4 components the attribute isn't split into two. On the other hand it's still possible to have two JointIds and two Weights for example, and have the second set in a different (e.g., less precise) type.

The builtin shader bindings slots stay four component, with the intention that (a) the shader is either compiled for an appropriate per-vertex component count, (b) a dynamic per-vertex component count is passed to it, or (c) the mesh is repacked to always contain the maximum number of components the shader expects.

A major change with the builtin attribute is that they have a flipped order compared to this PR -- JointIds and SecondaryJointIds are 6 and 10 instead of 7 and 11, Weights and SecondaryWeights are 7 and 11 instead of 6 and 10. This order made more sense to me, i.e. "joint i has a weight j" instead of "weight i is applied to joint j".

@mosra mosra closed this Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants