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

Regression for rendering between 1.117 and 1.118 #12041

Closed
javagl opened this issue Jun 19, 2024 · 3 comments · Fixed by #12043
Closed

Regression for rendering between 1.117 and 1.118 #12041

javagl opened this issue Jun 19, 2024 · 3 comments · Fixed by #12043

Comments

@javagl
Copy link
Contributor

javagl commented Jun 19, 2024

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 1.117...1.118 yet, but maybe someone already has an idea based on the description here.

@ggetz
Copy link
Contributor

ggetz commented Jun 20, 2024

@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
Copy link
Contributor

jjhembd commented Jun 20, 2024

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 #11970

@javagl
Copy link
Contributor Author

javagl commented Jun 20, 2024

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:

  • The specularFactor is constrained to minimum:0.0, maximum:1.0
  • The specularColorFactor does not have a maximum. I found that strange at the first glance, and considered that this might have been omitted accidentally, but...
  • Quickly skimming over the spec text brings up the Materials with reflectance parameter section, which says

    ... Typically, the reflectance ranges from 0% to 8%, given as a value in range [0,1], ...
    ...
    Therefore, by encoding an additional constant factor of 2 in specularColorFactor, we can convert from reflectance to specular color without any loss.

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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants