close
Skip to content

Gyroscope Bias Estimation - Unnecessary Max Operation#462

Closed
alainschoebi wants to merge 1 commit into
googlevr:masterfrom
alainschoebi:master
Closed

Gyroscope Bias Estimation - Unnecessary Max Operation#462
alainschoebi wants to merge 1 commit into
googlevr:masterfrom
alainschoebi:master

Conversation

@alainschoebi
Copy link
Copy Markdown

Summary
This pull request removes an unnecessary std::max operation in the gyroscope bias estimation code. This improves code readability and efficiency without impacting the functionality of the algorithm.

Description
The targeted code block checks if the gyroscope sample's norm exceeds a threshold (kGyroscopeForBiasThreshold). If it does, the function exits early; otherwise, it computes an update weight.

Here is the code block:

  if (gyroscope_sample_norm2 >= kGyroscopeForBiasThreshold) {
    return false;
  }

  float update_weight = std::max(
      0.0f, 1.0f - gyroscope_sample_norm2 / kGyroscopeForBiasThreshold);
  update_weight *= update_weight;

For the std::max operation to pick 0.0f over 1.0f - gyroscope_sample_norm2 / kGyroscopeForBiasThreshold, we need

0 >= 1 - gyroscope_sample_norm2 / kGyroscopeForBiasThreshold

which implies that

gyroscope_sample_norm2 >= kGyroscopeForBiasThreshold

However, this cannot occur due to the previous if-statement if (gyroscope_sample_norm2 >= kGyroscopeForBiasThreshold), which would exit the function before reaching this point.

This suggests that the max operation is unnecessary here and can be omitted for code readability and efficiency.

@alainschoebi alainschoebi changed the title gyroscope bias estimator removed max operation Gyroscope Bias Estimation - Unnecessary Max Operation Apr 18, 2024
@arilow
Copy link
Copy Markdown

arilow commented Apr 22, 2024

Thanks @alainschoebi for your pull request! Changes LGTM.

I ticket has been created to test your changes and merge as soon as we're able to do it internally.

@alainschoebi
Copy link
Copy Markdown
Author

Hi,

Thanks, @arilow. Do you have any updates on this? It appears that the merge has failed.

Best regards,
Alain

@arilow arilow added the cla: yes label Aug 8, 2024
@lneumarkt
Copy link
Copy Markdown

Your proposed changes have been already internally merged and will be included in the next release. Thanks for contributing to Cardboard!

@xinyunh0929
Copy link
Copy Markdown
Member

This was merged in v1.26.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: 1.26

Development

Successfully merging this pull request may close these issues.

4 participants