CesiumGS / cesium

An open-source JavaScript library for world-class 3D globes and maps :earth_americas:
https://cesium.com/cesiumjs/
Apache License 2.0
13.01k stars 3.51k forks source link

glTF 1.0: treat vec2 normals as oct-encoded to prevent crashes #10821

Open ptrgags opened 2 years ago

ptrgags commented 2 years ago

Recently we've seen quite a few cases where glTF 1.0 assets are crashing in CesiumJS 1.97 due to the NORMAL attribute being a vec2 rather than a vec3. In every case I've seen so far, this is because the glTF 1.0 technique oct-decodes the attribute in the vertex shader.

In Model, we don't do this, as we prefer things to use glTF 2.0 extensions. For example, with KHR_draco_mesh_compression, attributes can be quantized (positions) or oct encoded (normals). We handle this by creating a Quantization object in GltfLoader, and then the DequantizationPipelineStage handles this.

Talking with @lilleyse just now, here's one idea:

  1. Somewhere in GltfLoader (and/or GltfVertexBufferLoader if needed) if we come across an attribute from a glTF 1.0 with semantic = NORMAL, type = VEC2, do the following:
    1. look at the component type to determine the quantization range (e.g. it could be 16- or 32- bit)
    2. Generate a ModelComponents.Quantization object that describes oct-encoded normals. Include the quantization range. See the code for loading Draco attributes for an example.
  2. Try a failing model and verify that DequantizationPipelineStage handles it correctly.

CC @sanjeetsuhag

j9liu commented 2 years ago

This came up on the forum here: https://community.cesium.com/t/3dtiles-error-when-upgrade-cesium-to-1-97/20477

lilleyse commented 2 years ago

I started to fix this in the branch oct-encoded-normals but I'm having trouble finding test data. The old Power Plant model (asset id 8564) won't load out of the box for a different reason and the ISS model has VEC3 texcoords causing a separate crash. Are there any models I'm forgetting about?

lilleyse commented 2 years ago

There's also the NYC CityGML dataset in https://demos.cesium.com/NewYork/index.html (which has a download link)

The branch works except that lower sections of the buildings have slightly wrong normals. I haven't inspected the glTF close enough to know why. Local sandcastle demo.

Render Debug
image image
lilleyse commented 2 years ago

Here's one of the tiles with the shaders extracted: 0.zip

The oct decode function looks like czm_octDecode so I'm not sure what else could explain the artifacts.

ptrgags commented 2 years ago

To be more specific about the normal artifacts, When looking from above at the tops of buildings and rotating the view, the color changes from shades of magenta (+x, +z) to shades of blue-green (+x, +y), but never just blue (+z) like I would expect for normals in eye-coordinates.

2022-09-28_Abnormals

javagl commented 1 month ago

It may be worth noting that such glTF 1.0 files can sometimes be "upgraded" with the 3D Tiles Tools. A recent version added the required functionality. This is not available as a command-line functionality for a single file (i.e. there is no built-in upgradeGltf command or so). It is only applied to glTF 1.0 that is contained in B3DM that are part of a tileset, as part of the general upgrade --targetVersion 1.1 functionality. But if there is demand, offering a command for upgrading standalone glTF 1.0 files to 2.0 should be fairly trivial.