CesiumGS / cesium

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

Regression for rendering between 1.117 and 1.118 #12041

Closed javagl closed 4 months ago

javagl commented 5 months ago

A user provided a glTF model at https://community.cesium.com/t/measurment-tools-stopped-working/32962/12 When uploading this to Cesium ion and tiling it as 3D tiles, this is the result: Forum 32962 4x panel stojak as 3D Tiles.zip

Here is a comparison of the model rendered in 1.117 and 1.118:

Cesium Forum 32962 compare 117 to 118

The actual model is stored as a composite tile. For convenience/debugging/analyzing that, I extracted the GLB parts from that, including the JSON parts of the GLBs, attached here:

Forum 32962 composite elements.zip

The GLBs are using (Draco and) the rendering-related extensions

    "KHR_materials_specular",
    "KHR_materials_ior",

which might help to narrow down where the error was introduced. I didn't do a bisection or analysis of https://github.com/CesiumGS/cesium/compare/1.117...1.118 yet, but maybe someone already has an idea based on the description here.

ggetz commented 5 months ago

@jjhembd Can you please take a look at this (if you aren't already)? It would be great if we could fix this regression before the next release.

jjhembd commented 5 months ago

The model has the following value set in the KHR_materials_specular extension:

        "KHR_materials_specular": {
          "specularColorFactor": [
            2,
            2,
            2
          ]
        },

Before version 1.118, CesiumJS did not support this extension at all.

If I remove the specularColorFactor from the glTF and load in 1.118, the model renders as before: image

It is not immediately clear to me what the impact of the specularColorFactor should be in this case. But we could potentially have an error somewhere in https://github.com/CesiumGS/cesium/pull/11970

javagl commented 5 months ago

The KHR_materials_specular extension spec is... quite elaborate and math/shader-heavy, so I can not quickly say anything profound. A few things that caught my attention:

So that might explain where these 2, 2, 2 are coming from (and why there is no maximum: 1.0 ... even though I'd ask whether there should be a maximum: 2.0 then ...).

Maybe the possibility of these values being >1.0 is not taken into account properly.

(As a preliminary workaround, I posted a version of the model in the forum where the KHR_materials_specular extension was removed, just to at least have the option to render it properly (or at least "better") for now...)