Arbitrary number of skinning weights.#205
Arbitrary number of skinning weights.#205etherlore wants to merge 13 commits intofacebookincubator:mainfrom
Conversation
…d() on a Vec4f containing the weights. This normalizes the vector, i.e. makes the length of the vector equal to 1.0. For skinning weights what we want is the sum of the weights to be 1.0, which is a different. This commit fixes that.
…nningAccess to be agnostic to vertex types and support any number of bone influences. RawModel and Raw2Gltf still operates on (multiple) Vec4f Vec4i types for per vertex weights and bone ids. A good second step would be to generalize RawModel as well, though AddAttributeToPrimitive, GetAttributeArray and the attribute pointer in AttributeDefinition complicates this.
…ta structure to make code cleaner and simplify further generalization. Added array versions of AttributeDefinition and AddAttributeToPrimitive to facilitate this.
…d line parameter. Defaults to 4.
|
Thanks so much, will look this over later today. |
zellski
left a comment
zellski
left a comment
There was a problem hiding this comment.
Overall very nice work. I didn't investigate the actual logic yet; I figured I'd wait and see if you wanted to take care of the missing arrayIndex that fails compilation.
src/gltf/Raw2Gltf.hpp
Outdated
| glType(_glType), | ||
| dracoAttribute(dracoAttribute), | ||
| dracoComponentType(dracoComponentType), | ||
| arrayOffset(arrayOffset){} |
There was a problem hiding this comment.
I think the arrayOffset member is missing from AttributeDefinition? I could guess what it should be, but you are probably better equipped to fix it.
There was a problem hiding this comment.
Yes, I will do another fetch not that I should be able to get upstream thanks to your changes there.
src/raw/RawModel.cpp
Outdated
| if ((keep & RAW_VERTEX_ATTRIBUTE_JOINT_WEIGHTS) == 0) { | ||
| vertex.jointWeights = defaultVertex.jointWeights; | ||
| if ((keep & RAW_VERTEX_ATTRIBUTE_JOINT_INDICES) == 0) { | ||
| vertex.jointIndices = defaultVertex.jointIndices; |
There was a problem hiding this comment.
This is more of a nit, but generally indent should be 2 everywhere. It looks like you've got that down in most of your additions, but maybe just forgot here. The easiest way to make sure there aren't formatting mismatches is to set your editor to automatically run clang-format on every save... the included .clang-format file will ensure the source is on the exact right form.
There was a problem hiding this comment.
Ok, I will take a look and make sure we're good. We switched ourselves recently on some of our projects, so that may be why it's inconsistent in some of these changes.
src/raw/RawModel.hpp
Outdated
| Vec4f jointWeights{0.0f}; | ||
| std::vector<Vec4i> jointIndices; | ||
| std::vector<Vec4f> jointWeights; | ||
|
|
| "--draco-bits-for-normals", | ||
| gltfOptions.draco.quantBitsNormal, | ||
| "How many bits to quantize nornals to.", | ||
| "How many bits to quantize normals to.", |
| [&](std::vector<std::string> attributes) -> bool { | ||
| gltfOptions.keepAttribs = | ||
| RAW_VERTEX_ATTRIBUTE_JOINT_INDICES | RAW_VERTEX_ATTRIBUTE_JOINT_WEIGHTS; | ||
| RAW_VERTEX_ATTRIBUTE_JOINT_INDICES | RAW_VERTEX_ATTRIBUTE_JOINT_WEIGHTS; |
| raw.TransformTextures(texturesTransforms); | ||
| } | ||
| raw.Condense(); | ||
| raw.Condense(gltfOptions.maxSkinningWeights, gltfOptions.normalizeSkinningWeights); |
There was a problem hiding this comment.
Random thought: I'm starting to think we should have RawOptions, FbxOptions and GltfOptions interfaces, each one defined in the relevant subdirectory, then a concrete class that implements all three, and finally instantiate e.g. RawModel with an instance of the option class. It could be argued that it's more elegant to send to each method exactly the options it needs, but there's an equally valid point that once a method starts accepting more than 4 parameters, it's lost most of its ability to clean things up rather than muddy them.
(This method is obviously fine, just thinking outloud.)
There was a problem hiding this comment.
Yes, this was actually something I was considering as well, and influenced some of the design choices in the implementation. We could have gone a different route and sent options all the way to the gltf-agnostic Fbx2Raw stuff and had the Vec4 structures set up right there. But then I think it's good to keep that part of the code agnostic to gltf limitations and structures anyway.
|
I updated the PR with latest, and I believe I fixed the remaining issues. I hope this was done correctly, otherwise let me know and I can do better. If the commits are too messy I could do a fresh fork and apply the changes to that. Let me know what you think! |
|
@etherlore Can you review godotengine/FBX2glTF@6875e22 ? |
|
I updated the title to:
|
4 participants
This change allows for an arbitrary number skinning weights per vertex.
The number of weights allowed is controlled by the command line parameter --skinning-weights.
This defaults to 4 so as to not change the current behavior.
Normalization now also has a command line parameter --normalize-weights which also defaults to the current behavior of normalizing.
The entire pipeline has been upgraded to be agnostic to the number of weights per vertex. The Vec4 structures are populated at the end. Any remainder of weight/index space in the Vec4 structures are zeroed, again as the previous behavior. The code also determines the minimum set of Vec4s are globally for the mesh, and the command line parameter so as to not bloat with all zero structures. All vertices have the same number of Vec4s in the end, which I believe is what the spec requires.