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

WiimoteEmu: Expose IMU pointing accelerometer weight setting. #9147

Merged

Conversation

jordan-woyak
Copy link
Member

@jordan-woyak jordan-woyak commented Oct 12, 2020

Reducing this value can produce a less jittery pointer with reduced drift correction.

A user reported they had better results with 1% vs the default 2%.

The best name for the setting that I could come up with is "Accelerometer Influence".
I think it's descriptive and accurate enough. Anything else I can think if is ambiguous or too long.

@jordan-woyak jordan-woyak added the WIP / do not merge Work in progress (do not merge) label Oct 12, 2020
@iwubcode
Copy link
Contributor

Just based off your comment, I'm inclined to suggest "Accelerometer drift correction" or something to that effect.

And then maybe instead of a value, have a combo box with "high", "medium", "low" with some preset values. Otherwise I imagine users are going to crank it up to 100%.

// i18n: The symbol/abbreviation for percent.
_trans("%"),
// i18n: Refers to a setting controling the influence of accelerometer data.
_trans("Influence of accelerometer data on pitch and roll.")},

Choose a reason for hiding this comment

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

Consider instead "Accelerometer induced pitch-and-roll bias"

@rlnilsen
Copy link
Contributor

rlnilsen commented Oct 24, 2020

This setting is the ratio of how much trust you put in accelerometer vs gyroscope data (at least in my original code, I don't know if the new quaternion stuff is exactly equivalent). The gyroscope is accurate but drifty, the accelerometer is inaccurate but does not drift. So we mostly trust the gyroscope (98% by default), but use the accelerometer to correct gyroscope drift. So maybe "Accelerometer to gyroscope trust ratio"? I'm bad with names.

... I'm inclined to suggest "Accelerometer drift correction" or something to that effect.

Unlike gyroscopes, accelerometers do not have drift.
Ah, I see that this can read as both "correction of accelerometer drift" and "drift correction by accelerometer".

Copy link
Contributor

@rlnilsen rlnilsen left a comment

Choose a reason for hiding this comment

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

The code is untested by me but seems very straight forward and boiler plate, so once the wording issues are resolved this LGTM.

Edit: Gave it a spin and seems to be working well. The pointer gets pretty shaky already at 5%, so I don't know how useful it is to go above that though. This was with a DS4.

@rlnilsen
Copy link
Contributor

Perhaps just "Drift Correction" would be sufficient? That's really all one needs to draw the user's attention if they are experiencing drift. Then the tooltip can give a deeper explanation.

@leoetlino leoetlino marked this pull request as draft November 27, 2020 01:37
@JMC47
Copy link
Contributor

JMC47 commented Aug 1, 2021

I like the idea of drift correction for the user facing option name. If that's the only thing blocking this, we should probably change this and merge it.

@jordan-woyak
Copy link
Member Author

My issue with "Drift Correction" is it doesn't convey that accelerometer data is used.
In the future we might also have correction from IR data or whatever.
I'm leaning towards maybe "Accelerometer Coefficient" or "Accelerometer Weight".

@jordan-woyak jordan-woyak removed the WIP / do not merge Work in progress (do not merge) label Jun 13, 2022
@jordan-woyak jordan-woyak marked this pull request as ready for review June 14, 2022 04:19
@AdmiralCurtiss
Copy link
Contributor

Do you want to put this as an Advanced setting now that we have that?

@jordan-woyak
Copy link
Member Author

Do you want to put this as an Advanced setting now that we have that?

I'm thinking not. It's already on an "Advanced" tab.

@AdmiralCurtiss
Copy link
Contributor

Well alright then, this seems good otherwise, so...

@AdmiralCurtiss AdmiralCurtiss merged commit eccf527 into dolphin-emu:master Jul 7, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants