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 to prevent IndexOutOfRangeException #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paulsinnett
Copy link

This prevents the IndexOutOfRangeException exception #112 when Unity returns NaN for the refresh rate. It also displays the refresh rate with up to 3 decimal places so that rates like 29.97, 59.94, and 144.003 will show up correctly and not truncated.

This prevents the IndexOutOfRangeException exception when Unity returns NaN for the refresh rate. It also displays the refresh rate with up to 3 decimal places so that rates like 29.97, 59.94, and 144.003 will show up correctly and not truncated.
@paulsinnett
Copy link
Author

There are 3 problems (at least) wrapped up in this bug.

The index out of range happens due to the statement -value <= m_negativeBuffer.Length because if value is int.MinValue, then -value is also int.MinValue and the condition is always true. My change reverses the condition to value >= -m_negativeBuffer.Length and so can't suffer from this issue.

The next bug is why the function is getting int.MinValue. It is because we are converting a floating point refresh rate value to an integer before attempting to convert it to a string. My change avoids this conversion by directly converting it to a string. I have also cached this result so it doesn't allocate memory when the display data updates.

The final bug is due to Unity sometimes returning NaN for the refresh rate. That bug is discussed here: https://discussions.unity.com/t/refresh-rate-rounding-on-windows/887788/10

@paulsinnett
Copy link
Author

I have tested this on 2022 LTS and 2021 LTS for normal operation, and on a 2022 LTS AR Mobile Core project for the NaN case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant