-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
[Stay Aligned] Add small yaw rotations to keep trackers aligned #1293
base: main
Are you sure you want to change the base?
Conversation
i feel like this feature need deeper integration into the onboarding especially for the detect angles part. we cannot expect people to do it if we do not explain why and better explain how. Few things that we need to explain to the user precisely.
These questions also make me think that this could be integrated into autobone / done at the same time without user interraction. Also i would love a list of possible drawbacks: no solution is perfect and because this is basically guessing i suppose there is cases where this could underperfom |
Things like the adjustment rate, depending on the IMU type should be bound to the tracker settings. as people have been mismatching trackers imus in the past. also i feel like this should be auto detected / defaulted based on the IMU type |
I was thinking of building a wizard for walking the user through setting the angles. This would be a good place to address both of your questions. However, I am not a frontend engineer, so it's going to be challenging for me.
Auto detect angles should be done once, just like Autobone.
It should be done after full reset and mounting reset. It doesn't need Autobone specifically.
There is no step in Autobone to collect "relaxed" postures. Maybe this could be added as extra steps to Autobone? I don't have a strong opinion.
In all the situations I have tested, it outperforms the default by a lot, at least on BMI270s and BMI160s. Anecdotally, it also outperforms the default on BNO085s and LSM6DSVs (but I don't have those IMUs). I am not promising that it is perfect. For example, if you sit at a weird angle, you may see your body drift a little. But the drift ends up being capped to about 20 deg off-center, and the body proportions remain realistic. Compare this to the default where your chest or waist ends up twisted arbitrarily. I'm promising that it's better than the default. |
I would recommend just having one setting, chosen based on their worst IMU. Otherwise there'll be settings spread out across trackers, like "Enable drift compensation". In practice, 0.2 works well for all IMUs. I might even consider removing the setting completely. |
Butterscotch reminded me of a drawback. If the user sits in a significantly different position than what they selected in the relaxed body angles (e.g. they sit with their legs slightly crossed), Stay Aligned will try to undo their position. |
...er/core/src/main/java/dev/slimevr/tracking/processor/stayaligned/skeleton/TrackerSkeleton.kt
Outdated
Show resolved
Hide resolved
server/core/src/main/java/dev/slimevr/config/StayAlignedConfig.kt
Outdated
Show resolved
Hide resolved
This is a significant issue which will make it unusable in many social VR situations. Shouldn't it detect that the user is far from the position and just not apply alignment fix? |
I think the setting should be a multipler: x0.5, x1, x2. That's it. Being, 0.05deg/s, 0.1deg/s and 0.2deg/s. |
I played around with this idea a lot in 3.* and 4.* versions and came to the conclusion that it's better just to always adjust. If I try to limit it to a certain range, it quickly gets out of range due to larger player movements, and Stay Aligned stops working. |
@Eirenliel would it be OK if we addressed the UI stuff before release, or do I need to address it in this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I'm still not 100% on everything here, so I will do another more definitive review later. I like the structure developed here, should be easy to develop tests for in the future.
return ( | ||
<Typography color={color} whitespace="whitespace-nowrap"> | ||
{locked} {delta} {error} | ||
</Typography> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What these values mean should be explained somewhere, now that we have tooltips maybe we can use that?
import io.github.axisangles.ktmath.Vector3 | ||
import kotlin.math.* | ||
|
||
class Angle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be a value class
like our other math classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It becomes very annoying to access from Java if it's a value class
. I have to write a separate Java accessor for each angle I expose.
Also, I can't store both rad
and deg
, which means either:
- I rewrite the config classes to use raw floats; or
- I store ugly 0.199999 values in config
I'm happy to change it if you think it has to be a value class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to be, but it could be better that way, do we need to care about Java? You should still be able to store both rad and deg in that case? Maybe I'm misunderstanding value classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://kotlinlang.org/docs/inline-classes.html An inline class must have a single property initialized in the primary constructor. It cannot store multiple values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... let me make a separate DegreeAngle
class which will be used by config. It gets converted into an Angle
class for the rest of the process. Then Angle
can be an inline value class.
@@ -289,6 +289,8 @@ class TrackerResetsHandler(val tracker: Tracker) { | |||
yawResetSmoothTimeRemain = 0.0f | |||
} | |||
|
|||
tracker.stayAligned.reset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use a comment explaining its placement at the end of this function, it can seem inconsequential from a glance.
It's okay to address it in the later PR, but it shouldn't be this way on release, the settings are already confusing as is. |
Add small yaw rotations to keep trackers aligned. Stay Aligned applies small yaw corrects every server tick. It can successfully keep the user aligned in many poses including standing, sitting and lying down.
This feature has two primary ideas:
Every server tick, Stay Aligned picked a tracker, calculates an error based on 3 forces (locked trackers, centering force, and neighbor trackers), and uses gradient descent to nudge the tracker's yaw.
Main classes:
StayAligned
: Entry point of Stay Aligned that is called fromHumanSkeleton
AdjustTrackerYaw
: Performs the gradient descentLockedErrorVisitor
,CenterErrorVisitor
andNeighborErrorVisitor
calculates the errors for gradient descentStayAlignedTrackerState
: Tracks the Stay Aligned state per trackerSupporting classes:
TrackerSkeleton
: Visits trackers, while providing neighboring trackersTrackerPose
andTrackerSkeletonPose
: Detects the pose of the player from tracker orientationsRestDetector
: Detects when a tracker is at restTODO: For the actual release, we should make a wizard for detecting the relaxed body angles.
SlimeVR/SolarXR-Protocol#157