Normalize bone weights #185 #187
Conversation
|
Very cool, thanks @akulagrawal. Can you add some protection for divide-by-zero on |
src/COLLADA2GLTFWriter.cpp
Outdated
| @@ -1620,6 +1620,15 @@ bool COLLADA2GLTF::Writer::writeController(const COLLADAFW::Controller* controll | |||
| std::tie(type, joints, weights) = _skinData[skinControllerDataId]; | |||
| int numberOfComponents = GLTF::Accessor::getNumberOfComponents(type); | |||
|
|
|||
| float norm = 0.0; | |||
| for(int i = 0; i < weights.size(); i++) { | |||
| norm = norm + ( (*weights[i]) * (*weights[i]) ); | |||
There was a problem hiding this comment.
Normalization should be linear, not quadratic. The spec hasn't been updated on that yet.
There was a problem hiding this comment.
Oh cool I will change that to linear. @emackey sure, will do that.
There was a problem hiding this comment.
Hi @akulagrawal, thanks for the contribution! Just one small thing from me, please use size_t for your iterator instead of int. Otherwise, once the normalization is changed to linear this should be good to merge.
|
@lasalvavida I have updated everything. Please have a look now. |
src/COLLADA2GLTFWriter.cpp
Outdated
| @@ -1620,6 +1620,33 @@ bool COLLADA2GLTF::Writer::writeController(const COLLADAFW::Controller* controll | |||
| std::tie(type, joints, weights) = _skinData[skinControllerDataId]; | |||
| int numberOfComponents = GLTF::Accessor::getNumberOfComponents(type); | |||
|
|
|||
| float maxweight = 0.0, minweight = 0.0; | |||
| if (weights.size() > 0) { | |||
There was a problem hiding this comment.
I think this is redundant with the for loop below it and can probably be removed unless I'm missing something.
There was a problem hiding this comment.
Oh, I see why you did this; you can start the loop from 1 though so you don't do a redundant max/min on the first element.
There was a problem hiding this comment.
In my opinion, if this condition is not imposed, if in some case, weights.size() = 0, we will still end up trying to access 1st element of weights vector. But since there is no element in the weights vector, it would result in segmentation fault. A solution to this problem is to assign constant values to maxweight and minweight initially which are equal to the upper and lower bound of float datatype. But the current solution seemed to be more robust to me.
src/COLLADA2GLTFWriter.cpp
Outdated
| minweight = std::min(minweight, *weights[i]); | ||
| } | ||
| if (maxweight == minweight) { | ||
| if (maxweight == 0) { |
There was a problem hiding this comment.
I don't think you need to do this here. If max and min are both equal to zero, then that means all of the weights are already zero.
|
edit: Apologies, hold on a second while I edit this, it wasn't quite correct; it's been a while since I looked at this code |
|
The gist of my original comment was correct, but wasn't as clear as I think it could have been. Here goes again.
We want to normalize each set of weights independently. Currently this code does normalization on the first weight in each set across all sets. Does that make sense? |
|
For example, |
|
Oh yes! my bad. I just went with the literal meaning. Yeah I had a look at the code following the edit and |
|
Strange to find that sublime doesn't apply proper indentation. Indentation is fine in sublime but incorrect in reality. |
|
@lasalvavida @lexaknyazev @emackey please review it once more. Hope it's fine now |
|
Good work @akulagrawal! The code changes here look fine to me; I would just like for this to be tested on the CesiumMan COLLADA model rendered with log depth enabled to make sure that the issue is resolved. I should be able to take care of that this evening if no one does it before then. |
|
Tested in Cesium and can confirm that this resolves the issue, thanks @akulagrawal! |
Solved issue #185 by normalizing bone weights vector.