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

Fix acceleration axis and leg tweaks #1014

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

9il
Copy link
Contributor

@9il 9il commented Apr 29, 2024

Fixes #1006

This PR fixes the axis for leg tweaks! However, an additional update of thresholds may be required.

@ImUrX, please test it in standard mode with leg tweaks like skating correction.

I don't have slimes for testing.

image

@9il 9il force-pushed the fixAxis branch 7 times, most recently from 0f53831 to 9f6ff6a Compare April 29, 2024 21:13
@9il 9il changed the title [WIP] Fix acceleration axis Fix acceleration axis Apr 29, 2024
@9il 9il changed the title Fix acceleration axis Fix acceleration axis and leg tweaks Apr 29, 2024
@ImUrX ImUrX requested a review from Stermere April 29, 2024 21:26
@ImUrX ImUrX added Type: Bug Something isn't working Area: Hardware Protocol Related to communication with hardware/software trackers Area: Server Related to the server labels May 2, 2024
Copy link
Contributor

@Stermere Stermere left a comment

Choose a reason for hiding this comment

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

This won't affect skating-correction since the magnitude of acceleration is not being changed. The logic looks good as far as I can tell

@9il
Copy link
Contributor Author

9il commented May 9, 2024

This won't affect skating-correction since the magnitude of acceleration is not being changed.

Not really. Some code uses only the two components, like Vector3(footDif.x, 0f, footDif.z) . It uses x and z components, which correspond to trackers to original (x y) components in this PR and trackers (y z) in the current main branch. z in the tracker coordinates corresponds to the gravity direction. That is what the fix is about.

	private fun correctUnlockedFootTracker(footPosition: Vector3, previousFootPosition: Vector3, previousFootPositionCorrected: Vector3, footVelocity: Vector3, framesUnlocked: Int): Vector3 {
		var newFootPosition = footPosition
		var footDif = footPosition - previousFootPositionCorrected
		footDif = Vector3(footDif.x, 0f, footDif.z)

@Stermere
Copy link
Contributor

Stermere commented May 9, 2024

This won't affect skating-correction since the magnitude of acceleration is not being changed.

Not really. Some code uses only the two components, like Vector3(footDif.x, 0f, footDif.z) . It uses x and z components, which correspond to trackers to original (x y) components in this PR and trackers (y z) in the current main branch. z in the tracker coordinates corresponds to the gravity direction. That is what the fix is about.

	private fun correctUnlockedFootTracker(footPosition: Vector3, previousFootPosition: Vector3, previousFootPositionCorrected: Vector3, footVelocity: Vector3, framesUnlocked: Int): Vector3 {
		var newFootPosition = footPosition
		var footDif = footPosition - previousFootPositionCorrected
		footDif = Vector3(footDif.x, 0f, footDif.z)

That is a positional correction, the acceleration is not used like that, I just checked. Mocap mode does use the acceleration direction though so this will help that!

@Stermere Stermere self-requested a review May 9, 2024 20:49
@9il
Copy link
Contributor Author

9il commented May 18, 2024

@ImUrX is anything else required for this PR to be merged?

@ImUrX
Copy link
Member

ImUrX commented May 18, 2024

You need to wait for more reviewers

@9il
Copy link
Contributor Author

9il commented May 18, 2024

I though that two reviwers are enough. Should we ping someone to make a review?

Copy link
Member

@Erimelowo Erimelowo left a comment

Choose a reason for hiding this comment

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

code itself looks good but testing with official SlimeVR trackers, looking at the values on the GUI (not the preview), it still seems like the original way was the right way (Vector3(v.y, v.x, v.z)). Maybe it'd be worth looking in the firmware if the acceleration is transformed.

However, the accel arrows preview and the local -> global conversion for accel are both broken in some way

@9il
Copy link
Contributor Author

9il commented May 22, 2024

@Stermere id don't know if the fusion does the order right, but, at least, they in the right order during packet formation.

If you like I could take the original AXES_OFFSET and apply it both to rotation and acceleration. So, it will be defined by mostly by software, not a math.

On the other hand, if the bug is in the fustion, than the firmware has to be fixed instead of tuning the server for the firmware. At least, we could define a new version of protocol. The reason is because slime wants to be a kind of opensource standard for VR.

The current acceleratoin offsets is defenetely wrong. Some code may work better now because it was tuned for the current wrong offsets. That could work for now but it can be so for a robust code evolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Hardware Protocol Related to communication with hardware/software trackers Area: Server Related to the server Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Acceleration decoding doesn't respect AXES_OFFSET
5 participants