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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Check for NaN in Camera::world_to_screen() #3268

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Check for NaN in Camera::world_to_screen() #3268

wants to merge 4 commits into from

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Dec 6, 2021 •
edited
Loading

Objective

  • Checks for NaN in computed NDC space coordinates, fixing unexpected NaN in a fallible (Option<T>) function.

Solution

  • Adds a NaN check, in addition to the existing NDC bounds checks.
  • This is a helper function, and should have no performance impact to the engine itself.
  • This will help prevent hard-to-trace NaN propagation in user code, by returning None instead of Some(NaN).

Depends on #3269 for CI error fix.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 6, 2021
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Dec 6, 2021
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 6, 2021
@mockersf
Copy link
Member

mockersf commented Dec 6, 2021

CI error should be fixed (#3269) if you rebase on main

@mockersf
Copy link
Member

mockersf commented Dec 6, 2021 •
edited
Loading

This will help prevent hard-to-trace NaN propagation in user code, by returning None instead of Some(NaN).

The NaN wasn't propagated, you'r checking for it on the z component which is discarded after. Do you know how it would be possible to have this case happens?

mockersf
mockersf requested changes Dec 6, 2021
View reviewed changes
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

Could you also do the same fix on pipelined/bevy_render2/src/camera/camera.rs?

@mockersf mockersf removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 6, 2021
@aevyrie
Copy link
Member Author

aevyrie commented Dec 6, 2021 •
edited
Loading

This will help prevent hard-to-trace NaN propagation in user code, by returning None instead of Some(NaN).

The NaN wasn't propagated, you'r checking for it on the z component which is discarded after. Do you know how it would be possible to have this case happens?

This PR is the result of a bug I found while debugging an application. The NaN was propagated in the Vec2 this function returns. I found that in all cases, inputs that resulting in the Vec2 containing NaNs, could be caught by checking the z value alone, which we are already using for comparison in these other checks. I don't know if this is provable for all cases.

Could you also do the same fix on pipelined/bevy_render2/src/camera/camera.rs?

Good call, will do.

@aevyrie
Copy link
Member Author

aevyrie commented Dec 6, 2021

@mockersf I've added a fix to pipelined, and changed the check to use is_nan on the output Vec2.

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 6, 2021
@cart
Copy link
Member

cart commented Dec 8, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 8, 2021
@aevyrie
Check for NaN in Camera::world_to_screen() (#3268)
38c7d5e 
# Objective

- Checks for NaN in computed NDC space coordinates, fixing unexpected NaN in a fallible (`Option<T>`) function.

## Solution

- Adds a NaN check, in addition to the existing NDC bounds checks.
- This is a helper function, and should have no performance impact to the engine itself.
- This will help prevent hard-to-trace NaN propagation in user code, by returning `None` instead of `Some(NaN)`.


Depends on #3269 for CI error fix.
@bors
Copy link
Contributor

bors bot commented Dec 8, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Check for NaN in Camera::world_to_screen() [Merged by Bors] - Check for NaN in Camera::world_to_screen() Dec 8, 2021
@bors bors bot closed this Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@IceSentry IceSentry IceSentry approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aevyrie @mockersf @cart @alice-i-cecile @IceSentry