Skip to content

OneEuroFilter incorrectly translates from duration to frequency. #4316

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

Closed
1 of 21 tasks
peteams opened this issue Oct 13, 2021 · 2 comments · Fixed by #4317
Closed
1 of 21 tasks

OneEuroFilter incorrectly translates from duration to frequency. #4316

peteams opened this issue Oct 13, 2021 · 2 comments · Fixed by #4317
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥 gaze 👀
Milestone

Comments

@peteams
Copy link
Contributor

peteams commented Oct 13, 2021

Describe the bug

The GazeInteraction library implementation of OneEuroFilter is broken. The conversion from C++ to C# broke a calculation between time and frequency. The 100000000.0f (1e8) in the following line should be 10000000.0f (1e7).

This:

float samplingFrequency = 100000000.0f / Math.Max(1, (args.Timestamp - _lastTimestamp).Ticks);

Is better expressed as:

var samplingFrequency = (float)TimeSpan.TicksPerSecond / Math.Max(1, (args.Timestamp - _lastTimestamp).Ticks);

  • Is this bug a regression in the toolkit? If so, what toolkit version did you last see it work:

The calculation was correct in the C++ implementation (probably).

Steps to Reproduce

Error was found during a code inspection; the actual code is not used in the current implementation of GazeInteraction, though it is theoretically available for consuming code to use.

  • Can this be reproduced in the Sample App? (Either in a sample as-is or with new XAML pasted in the editor.) If so, please provide custom XAML or steps to reproduce. If not, let us know why it can't be reproduced (e.g. more complex setup, environment, dependencies, etc...)

Steps to reproduce the behavior:

  1. Given the following environment (Sample App w/ XAML, Project with Isolated setup, etc...)
  2. Go to '...'
  3. Click on '....'
  4. Scroll down to '....'
  5. See error

Expected behavior

Screenshots

Environment

NuGet Package(s):

Package Version(s):

Windows 10 Build Number:

  • Fall Creators Update (16299)
  • April 2018 Update (17134)
  • October 2018 Update (17763)
  • May 2019 Update (18362)
  • May 2020 Update (19041)
  • Insider Build ({build_number})

App min and target version:

  • Fall Creators Update (16299)
  • April 2018 Update (17134)
  • October 2018 Update (17763)
  • May 2019 Update (18362)
  • May 2020 Update (19041)
  • Insider Build ({build_number})

Device form factor:

  • Desktop
  • Xbox
  • Surface Hub
  • IoT

Visual Studio version:

  • 2017 (15.{minor_version})
  • 2019 (16.{minor_version})
  • 2022 (17.{minor_version})

Additional context

@peteams peteams added the bug 🐛 An unexpected issue that highlights incorrect behavior label Oct 13, 2021
@ghost ghost added the needs triage 🔍 label Oct 13, 2021
@ghost
Copy link

ghost commented Oct 13, 2021

Hello peteams, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

@azchohfi
Copy link
Contributor

Ops. Thats on me. Thanks for reporting it!

michael-hawker added a commit that referenced this issue Oct 14, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fix to issue #4316 - error in duration to frequency calculation.
@ghost ghost added Completed 🔥 and removed In-PR 🚀 labels Oct 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior Completed 🔥 gaze 👀
Projects
None yet
3 participants