Skip to content

Conversation

@greeble-dev
Copy link
Contributor

@greeble-dev greeble-dev commented Aug 14, 2025

Objective

Fix glTF coordinate conversion not converting tangents.

image

Report: https://discord.com/channels/691052431525675048/692572690833473578/1405362252617355335
Thread: https://discord.com/channels/691052431525675048/1405451520836898848

Solution

Fixed by removing a redundant copy of the tangent attributes.

In #5370 attribute copying was moved to a more generic function (see convert_attribute). But one code path was left behind, so the tangents are actually copied twice. This is technically a bug, but a harmless one since both copies are the same.

Later on, #19633 added the option to convert attribute coordinates. But only the first tangent copy (in convert_attribute) applies the conversion - the second copy will overwrite them with unconverted values. This PR removes the second copy.

There's also some minor code quality tweaks - prefer contains_attribute() to attribute().is_some(), and fixed some bad indentation in log macros. I can revert these if we want to play it safe.

Testing

Tested a modified version of example load_gltf with and without feature gltf_convert_coordinates_default. Also tested after hacking the loader to use the code path where tangents are recalculated.

@janhohenheim janhohenheim added D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-glTF Related to the glTF 3D scene/model format labels Aug 14, 2025
@janhohenheim janhohenheim added this to the 0.17 milestone Aug 14, 2025
@janhohenheim
Copy link
Member

Adding to the milestone because this is needed for the glTF bonanza

@greeble-dev greeble-dev added C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 14, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 14, 2025
Merged via the queue into bevyengine:main with commit cbe1e02 Aug 14, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-glTF Related to the glTF 3D scene/model format C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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

Development

Successfully merging this pull request may close these issues.

4 participants