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

Decode oct-encoded normals from glTF 1.0 #98

Merged
merged 4 commits into from
Feb 5, 2024
Merged

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Feb 4, 2024

Fixes #97

Details are described in the issue and the comment linked from there.
A tl:dr:

  • glTF 1.0 (in B3DM or I3DM) could contain oct-encoded (VEC2/BYTE or VEC2/SHORT) normals
  • in glTF 1.0, these had been decoded in the shader (that was part of glTF 1.0)
  • When upgrading glTF 1.0 to glTF 2.0 with gltf-pipeline, then the 2D normals are transferred into the glTF asset
  • This caused an invalid asset (e.g. with rendering errors in CesiumJS)

With this PR, these 2D normals will be converted into 3D normals as part of the upgrade --targetVersion 1.1 step.


The upgrade --targetVersion 1.1 will convert B3DM and I3DM to GLB. After the GLB was upgraded from glTF 1.0 to 2.0, the upgrade step now creates a glTF-Transform document, goes through all meshes/primitives/NORMAL attributes, and if one of them is a VEC2/BYTE or VEC2/SHORT accessor, then it will be converted into 3D normals by oct-decoding them (with a snippet borrowed from CesiumJS octDecode.glsl).

I have added a spec file in the new specs/data/migration/input/BatchedLegacy/BatchedWithGltf1With2DNormals/ directory. Creating such an "invalid" glTF 1.0 was a bit of a hassle, but ... https://github.com/javagl/JglTF still supports glTF 1.0, so that was doable. The B3DM that contains this glTF 1.0 will be converted into a glTF 2.0 GLB (and the JSON of that is used as the 'golden' reference, similar to the other migration specs).

I also tested the whole thing with the 'New York' data that originally sparked the linked issue. (I could probably just have used a B3DM from that data set, but ... who knows that the licensing implications could be. I'm trying really hard to stay away from lawyers). The resulting data set can be rendered in CesiumJS as expected.

@lilleyse lilleyse merged commit 3464f07 into main Feb 5, 2024
2 checks passed
@lilleyse lilleyse deleted the upgrade-gltf1-2d-normals branch February 5, 2024 14:36
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.

Fix legacy oct-encoded normals in glTF as part of upgrade
2 participants