Skeleton Constraints #1222
Skeleton Constraints #1222
Conversation
…ct later chain's ability to reach the target.
…le to solve due to join restrictions
Stermere
requested review from
ButterscotchV,
Eirenliel,
Erimelowo,
ImUrX and
loucass003
as code owners
github-actions
bot
added
Area: Skeletal Model
Stermere
added
Priority: Normal
ButterscotchV
added
the
Status: Blocked by other issue
| val leftUpperShoulderBone = Bone(BoneType.LEFT_SHOULDER, Constraint(ConstraintType.COMPLETE)) | ||
| val rightUpperShoulderBone = Bone(BoneType.RIGHT_SHOULDER, Constraint(ConstraintType.COMPLETE)) |
There was a problem hiding this comment.
Should this be upper shoulder BoneTypes?
| val leftUpperShoulderBone = Bone(BoneType.LEFT_SHOULDER, Constraint(ConstraintType.COMPLETE)) | |
| val rightUpperShoulderBone = Bone(BoneType.RIGHT_SHOULDER, Constraint(ConstraintType.COMPLETE)) | |
| val leftUpperShoulderBone = Bone(BoneType.LEFT_UPPER_SHOULDER, Constraint(ConstraintType.COMPLETE)) | |
| val rightUpperShoulderBone = Bone(BoneType.RIGHT_UPPER_SHOULDER, Constraint(ConstraintType.COMPLETE)) |
| @@ -62,6 +62,7 @@ class TrackerResetsHandler(val tracker: Tracker) { | |||
| var mountRotFix = Quaternion.IDENTITY | |||
| private set | |||
| private var yawFix = Quaternion.IDENTITY | |||
| private var dynamicFix = Quaternion.IDENTITY | |||
There was a problem hiding this comment.
Perhaps this name could be more descriptive, like constraintFix? The current name is vague and could refer to a variety of features.
| tracker?.filteringHandler?.getFilteringImpact() | ||
| ?: 1f | ||
| ) < 0.01f && ( | ||
| parentTracker?.filteringHandler?.getFilteringImpact() | ||
| ?: 1f | ||
| ) < 0.01f |
There was a problem hiding this comment.
This feels a bit restrictive? I'm not sure what values we can regularly expect to see for filtering impact. I would like to have some feedback about what values should be expected here if possible.
| parentTracker?.filteringHandler?.getFilteringImpact() | ||
| ?: 1f |
There was a problem hiding this comment.
The parent tracker may be null in cases like having a hip tracker with no waist tracker, this condition would prevent any correction in that case despite being the most common set-up. imo the parent tracker should be considered 0f (or -1f to be safe?) if not present.
| parentTracker?.filteringHandler?.getFilteringImpact() | |
| ?: 1f | |
| parentTracker?.filteringHandler?.getFilteringImpact() | |
| ?: 0f |
| bone.setRotationRaw(newRot) | ||
| bone.updateThisNode() |
There was a problem hiding this comment.
If we want to apply each transform here in this way, this needs to be run top-down through the hierarchy, otherwise the child bones to the current bone will not inherit the position computed by the current bone.
Perhaps this method of constraints should be reconsidered to be applied within the bones (or nodes?) themselves rather than in the skeleton. Correction could then be handled as an output of the bone, as it shouldn't matter when we apply the correction, as whenever we apply it, it should return to a value of 0 if done correctly.
| val parent = thisBone.parent!! | ||
| val localRotationOffset = parent.rotationOffset.inv() * thisBone.rotationOffset | ||
| val rotationLocal = | ||
| (parent.getGlobalRotation() * localRotationOffset).inv() * rotation |
There was a problem hiding this comment.
This code doesn't make sense being here and it is duplicated between constraint functions, perhaps this would do better in the Bone class. I could see this being used in getLocalRotation() if the TransformNode is TransformNode.localRotation == false? I would like an opinion on this.
| } | ||
|
|
||
| private fun constrain(rotation: Quaternion, angle: Float): Quaternion { | ||
| val magnitude = sin(angle * 0.5f) |
There was a problem hiding this comment.
I would like a comment explaining this if possible, I do not understand why sine is used here or why we multiply by 0.5f. What is the magnitude of?
|
|
||
| // Constraint function for CompleteConstraint | ||
| private val completeConstraint: ConstraintFunction = { _: Quaternion, thisBone: Bone, _: Float, _: Float, _: Float -> | ||
| thisBone.getGlobalRotation() |
There was a problem hiding this comment.
Should we not return the initial rotation provided? I think using the bone is beyond the necessary scope.
| * Apply rotational constraints and if applicable force the rotation | ||
| * to be unchanged unless it violates the constraints | ||
| */ | ||
| fun applyConstraint(rotation: Quaternion, thisBone: Bone): Quaternion = constraintFunction(rotation, thisBone, swingRad, twistRad, allowedDeviationRad).unit() |
There was a problem hiding this comment.
Perhaps we could remove Bone from the scope of these constraints entirely and replace rotation with localRotation: Quaternion as the current bone's local rotation and thisBone could be replaced with parentRotation: Quaternion as the parent bone's global rotation? I don't think any of the constraints use anything else besides COMPLETE, but that could be handled in another way (or maybe we just take current global rotation instead? it could also be handled earlier in the helper functions, see below).
We could instead use helper functions to accept a global current rotation and a Bone, that way we have minimal input for constraints. This would be beneficial for unit testing and reduce complexity and duplication.
| fun updateThisNode() { | ||
| headNode.updateThisNode() | ||
| tailNode.updateThisNode() | ||
| } |
There was a problem hiding this comment.
This is unsafe in regards to the bone hierarchy. Please see my earlier comment in HumanSkeleton regarding a refactor of how constraints are applied (#1222 (comment)).
Adds constraints to the joints of the skeleton allowing for unrealistic joint angles to be clamped to more reasonable values and for extreme drift to be compensated for. Needs SlimeVR/SolarXR-Protocol#152 to get merged