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 sanitizers reports about octahedral tangents in RenderingServer #78902

Conversation

nklbdev
Copy link
Contributor

@nklbdev nklbdev commented Jul 1, 2023

I found three places in RenderingServer with wrong type casting from float clamped by range [0..65535] to int to int16_t[-32768..32767] instead uint16_t[0..65535].
In rarely situations it leads conversion from big positive number to negative.
In some cases it can break the check Linux / Editor with doubles and GCC sanitizers.

@nklbdev nklbdev requested a review from a team as a code owner July 1, 2023 08:41
@akien-mga akien-mga changed the title fix wrong type Fix wrong type casting for octahedral tangents Jul 1, 2023
@akien-mga
Copy link
Member

Looks great! Could you update the commit message to be more explicit (e.g. like I did with the PR title)?

Would this happen to be this issue? #78749

@RedworkDE
Copy link
Member

RedworkDE commented Jul 1, 2023

No, that issue is about a free without malloc / memory corruption issue, while the linked CI fail is an index out of bounds access to a vector.

@nklbdev nklbdev force-pushed the Fix_wrong_type_casting_in_RenderingServer branch from 032f418 to c022f52 Compare July 1, 2023 10:16
@nklbdev
Copy link
Contributor Author

nklbdev commented Jul 1, 2023

Looks great! Could you update the commit message to be more explicit (e.g. like I did with the PR title)?

I fixed the commit message as you requested.))

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

LGTM.

@akien-mga
Copy link
Member

akien-mga commented Jul 1, 2023

For the record, even when the CI step doesn't error out, the following errors are reported which relate to the code here:

servers/rendering_server.cpp:406:21: runtime error: nan is outside the range of representable values of type 'short int'
servers/rendering_server.cpp:407:21: runtime error: nan is outside the range of representable values of type 'short int'
servers/rendering_server.cpp:426:22: runtime error: nan is outside the range of representable values of type 'short int'
servers/rendering_server.cpp:427:22: runtime error: 32768.5 is outside the range of representable values of type 'short int'

This change fixes the last one, but the first three errors about nan are still raised. Could be interesting to look into in a follow-up PR, or this one if you want and the fix is similarly trivial.

@nklbdev
Copy link
Contributor Author

nklbdev commented Jul 1, 2023

Okay, now I'll try to find the causes of the rest of the errors.

@nklbdev nklbdev requested a review from a team as a code owner July 1, 2023 13:47
@nklbdev
Copy link
Contributor Author

nklbdev commented Jul 1, 2023

I think I've found the cause of the rest of the errors, but I'm not sure if I substituted the correct values when forming the mesh. Please correct me as I haven't worked much with 3D scenes and coordinates.

I found the problem place by setting a breakpoints with a conditions, and then examining the call stack

@clayjohn clayjohn added this to the 4.2 milestone Jul 1, 2023
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 6, 2023
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

This looks great!

The defaults in SpriteBase3D just need to be normalized. They won't be used as-is as the values are always overwritten, so moving from 0,0,0 to any normalized vector is fine.

@nklbdev
Copy link
Contributor Author

nklbdev commented Jul 6, 2023

This looks great!

The defaults in SpriteBase3D just need to be normalized. They won't be used as-is as the values are always overwritten, so moving from 0,0,0 to any normalized vector is fine.

I'm very happy) Does this mean that the current state of the code is valid?

@clayjohn
Copy link
Member

clayjohn commented Jul 6, 2023

This looks great!
The defaults in SpriteBase3D just need to be normalized. They won't be used as-is as the values are always overwritten, so moving from 0,0,0 to any normalized vector is fine.

I'm very happy) Does this mean that the current state of the code is valid?

Yes, as far as I can tell!

@akien-mga akien-mga merged commit b8f28e2 into godotengine:master Jul 7, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 10, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.1.

@akien-mga akien-mga changed the title Fix wrong type casting for octahedral tangents Fix sanitizers reports about octahedral tangents in RenderingServer Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants