Some more fixes for compressed meshes #83704
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
clayjohn
commented
Oct 20, 2023
| @@ -1399,7 +1397,7 @@ Array RenderingServer::_get_array_from_surface(uint64_t p_format, Vector<uint8_t | |||
| tangentsw[j * 4 + 3] = tan.w; | |||
| } | |||
| ret[RS::ARRAY_NORMAL] = normals; | |||
| ret[RS::ARRAY_FORMAT_TANGENT] = tangents; | |||
| ret[RS::ARRAY_TANGENT] = tangents; | |||
There was a problem hiding this comment.
This alone fixes #83526. What a silly mistake
clayjohn
force-pushed
the
misc-mesh-fixes
branch
from
1df09a7
to
75eefc7
Compare
akien-mga
approved these changes
Oct 21, 2023
AThousandShips
approved these changes
Oct 21, 2023
|
I looked at these compressed mesh fixes, but I wasn't able to see anything obviously wrong. |
bitsawer
reviewed
Oct 21, 2023
clayjohn
force-pushed
the
misc-mesh-fixes
branch
from
75eefc7
to
805ffa4
Compare
|
Updated based on comments! |
akien-mga
reviewed
Oct 23, 2023
This cleans up a few more cases of uint32_t->uint64_t Importantly this fixes an edge case in the axis-angle compression by using the pre-existing Basis methods instead
clayjohn
force-pushed
the
misc-mesh-fixes
branch
from
805ffa4
to
8f9cd4e
Compare
|
Fixed up the error message and rebased! |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This cleans up a few more cases of uint32_t->uint64_t
Importantly this fixes an edge case in the axis-angle compression by using the pre-existing Basis methods instead
Fixes: #83240
Fixes: #83526
This might help #83667, but I wasn't able to reproduce, so I don't know for sure.
For posterity, most of the changes here are just cleaning up uint32_t->uint64_t and other QoL changes I caught while debugging. The biggest change is the change to
_get_axis_angle(). Instead of using the small helper function, I switched to usingBasis::_get_axis_angle()which has the proper check for if the matrix is symmetrical. The issue we were running into was that some meshes contain bogus tangents (#83240 is full of them). This can happen with meshes that don't have UVs set up for normalmapping and have the "generate_tangents" flag checked by default.Sfter spending hours trying to understand the math behind the compression and find any source for the original code. I stumbled on https://en.wikipedia.org/wiki/Rotation_matrix#Conversion_from_rotation_matrix_to_axis%E2%80%93angle which explains everything perfectly and derives the steps exactly how we have them laid out in the code. It contains this important warning:
In searching for
is_symmetric(), I founddiagonalize()which led me toBasis::_get_axis_angle()which contained all the proper checks and cleaning to properly handle bogus tangents. Accordingly, I switched over to using Basis entirely and everything works nicely.