fix for converting KHR_materials_common CONSTANT technique to KHR_materials_unlit

[deng0]

issue: https://github.com/CesiumGS/cesium/issues/11821

[javagl]

I have some thoughts about the structure of the convertMaterialsCommonToPbr function in general that are unrelated to the change here. For example: When a function body has the form

function example() {
    if (haveToDoSomething) {
        // 100 lines of code
    }
}

then I'm usually leaning towards the pattern of

function example() {
    if (!haveToDoSomething) {
        return;
    }
    // 100 lines of code
}

just to keep indentations at bay and not having to scroll down to a potential else, but some of that may be subjective....

---

However, regarding the current change:

The ambient/diffuse/emission variables became undeclared by this change:

Now you could slap in some lets there, but what I don't like about the current solution is that it later says diffuse = emission. This might be confusing in the subsequent code. For example, the diffuse factor was converted with srgbToLinear, while the emission was not. The spec isn't really clear about this point, though, so one might have to make guesses and assumptions here.

A suggested fix/implementation could be

function assignAsBaseColor(material, baseColor) {
  if (defined(baseColor)) {
    if (isVec4(baseColor)) {
      material.pbrMetallicRoughness.baseColorFactor = srgbToLinear(baseColor);
    } else if (isTexture(baseColor)) {
      material.pbrMetallicRoughness.baseColorTexture = baseColor;
    }
  }
}

function assignAsEmissive(material, emissive) {
  if (defined(emissive)) {
    if (isVec4(emissive)) {
      material.emissiveFactor = emissive.slice(0, 3);
    } else if (isTexture(emissive)) {
      material.emissiveTexture = emissive;
    }
  }
}

function convertMaterialsCommonToPbr(gltf) {
  // Future work: convert KHR_materials_common lights to KHR_lights_punctual
  ForEach.material(gltf, function (material) {
    const materialsCommon = defaultValue(
      material.extensions,
      defaultValue.EMPTY_OBJECT,
    ).KHR_materials_common;
    if (!defined(materialsCommon)) {
      // Nothing to do
      return;
    }

    const values = defaultValue(materialsCommon.values, {});
    const ambient = values.ambient;
    const diffuse = values.diffuse;
    const emission = values.emission;
    const transparency = values.transparency;

    // These actually exist on the extension object, not the values object despite what's shown in the spec
    const doubleSided = materialsCommon.doubleSided;
    const transparent = materialsCommon.transparent;

    // Ignore specular and shininess for now because the conversion to PBR
    // isn't straightforward and depends on the technique
    initializePbrMaterial(material);

    const technique = materialsCommon.technique;
    if (technique === "CONSTANT") {
      // Add the KHR_materials_unlit extension
      addExtensionsUsed(gltf, "KHR_materials_unlit");
      material.extensions = defined(material.extensions)
        ? material.extensions
        : {};
      material.extensions["KHR_materials_unlit"] = {};

      // The CONSTANT technique does not support 'diffuse', so
      // assign either the 'emission' or the 'ambient' as the
      // base color
      assignAsBaseColor(material, emission);
      assignAsBaseColor(material, ambient);
    } else {
      // Assign the 'diffuse' as the base color, and
      // the 'ambient' or 'emissive' as the emissive
      // part if they are present.
      assignAsBaseColor(material, diffuse);
      assignAsEmissive(material, ambient);
      assignAsEmissive(material, emission);
    }

    if (defined(doubleSided)) {
      material.doubleSided = doubleSided;
    }
    if (defined(transparency)) {
      if (defined(material.pbrMetallicRoughness.baseColorFactor)) {
        material.pbrMetallicRoughness.baseColorFactor[3] *= transparency;
      } else {
        material.pbrMetallicRoughness.baseColorFactor = [1, 1, 1, transparency];
      }
    }
    if (defined(transparent)) {
      material.alphaMode = transparent ? "BLEND" : "OPAQUE";
    }
  });

  removeExtension(gltf, "KHR_materials_common");
}

An example is in https://github.com/CesiumGS/gltf-pipeline/commit/fe58a4b767332f36bc19db9bb7e71f20f77509d3 . If you want to update your PR so that the variables are declared, and maybe add a test case, then it could probably be merged. Otherwise, I'd open a PR for that linked change.

[javagl]

(BTW: I have not yet tried the suggested fix for the model that you provided in the issue, but can do this if necessary - I think the result should be the same...)

[deng0]

@javagl thanks for looking into the issue and the PR.

I've tested your version of the fix and as expected it also results in the model being rendered correctly.

My quick fix was mostly about checking whether the problem is resolved with the least lines changed. Your fix looks cleaner. Feel free to create a PR for your commit. I will close this one.