KHR_materials_common and KHR_binary_glTF compatibility

[mlimper]

(currently not intended for direct merge, just for discussion)

Hi Cesium folks! :-)

After writing a binary glTF exporter, also using the KHR_materials_common extension, this PR summarizes my changes to Cesium, in order to get it working with the glb files I was exporting.
It also lists some open points regarding the spec.

Here's what I noticed (items numbered to ease discussion):

1. The current Cesium glb example with KHR_materials_common uses objects as parameters ("ambient", "diffuse", etc.), having each a "type" and "value" member. According to the spec, there should only be direct values, no extra object per parameter (see example here). This is because the types are already pre-defined in the spec, so I added a function to determine types via parameter name (here).
2. I ran into a problem with parameters being interpreted as shader params, although the boolean flags ("transparent" and "doubleSided") are not meant to be used in the shader, I guess - so I added an additional check (here). Do you think this is correct, and would this be the right way to deal with this issue?
3. The binary glTF spec says the start of the body should be aligned to 4-byte-boundaries. I think we figured out at some point that this might not be necessary if you just want to push your data to the GPU, since you would then just use Uint8Array view types. Cesium does not seem to use this assumption any more, so I had to add some code to make it work with data that has additional padding to fulfill the alignment requirement (see here). Nevertheless, this is not a very elegant solution, it makes reader implementations quite error-prone. If we want the alignment requirement and don't want to change the spec, it seems the most robust solution would be right now to not use an explicit padding, but instead make the writer add whitespace (or so) to the header, so that the start of the body becomes 4-byte-aligned. Does it make sense to remove that passage from the spec again? I guess it only makes sense if you want to use some Float32 arrays directly in JS... (last discussion post here) If we decide it still makes sense to have this alignment, we should provide a hint to implementers where / how to deal with that (writer side / reader side).
4. It seems Cesium (and the writer that was used for the examples) calls the special buffer "KHR_binary_glTF", but the spec calls it just "binary_glTF". One of both should probably be adjusted.
5. With the KHR_materials_common extension, there are no techniques / shaders that must be defined - I like that a lot :-) It enables usage across systems and shader languages. However, this requires you even more to adhere to a fixed naming of the vertex attributes, since their semantics are defined nowhere when the KHR_materials_common extension is used. In the case of our own rendering system, we used "position" and "normal", for example, whereas I had to export using "POSITION" and "NORMAL" to make the models work with Cesium. I would vote against any over-generalization to fix this issue (that would just make implementations more complex error-prone) and instead just propose to nail down a set of common attribute names for usage with KHR_materials_common. What do you think?
6. In parts related to the last point: it seems a bit odd that the bounding box of the model must be indirectly defined via the POSITION attribute. It would probably be very useful to have (optional) bounding box definitions for "mesh" and / or "primitive". One reason is that one could use custom shaders / attributes that encode the vertex positions differently (see WEB3D_quantized_attributes extension), and therefore have min/max values that differs from the bounding volume of the respective primitive / mesh.

Well, I guess that's a lot of input for discussion, and some of the points are clearly more related to glTF than to Cesium... still, I believed it could be useful to collect everything that came up during this test in one place.

Thanks a lot in advance for your input :-)

[pjcozzi]

Thanks @mlimper! Your comments and code are useful. Can you please send in a CLA so we can merge whatever the final code changes turn out to be?

[pjcozzi]

1. The current Cesium glb example with KHR_materials_common uses objects as parameters ("ambient", "diffuse", etc.), having each a "type" and "value" member. According to the spec, there should only be direct values, no extra object per parameter (see example here). This is because the types are already pre-defined in the spec, so I added a function to determine types via parameter name (here).

I believe the models and Cesium code are right and that the extension spec is wrong (well, not up to date). This was discussed starting with this comment: KhronosGroup/glTF#424 (comment). Do you want to open a pull request to update the extension in the glTF repo?

[pjcozzi]

1. I ran into a problem with parameters being interpreted as shader params, although the boolean flags ("transparent" and "doubleSided") are not meant to be used in the shader, I guess - so I added an additional check (here). Do you think this is correct, and would this be the right way to deal with this issue?

@tfili what do you think?

[pjcozzi]

3 . The binary glTF spec says the start of the body should be aligned to 4-byte-boundaries. I think we figured out at some point that this might not be necessary if you just want to push your data to the GPU, since you would then just use Uint8Array view types. Cesium does not seem to use this assumption any more, so I had to add some code to make it work with data that has additional padding to fulfill the alignment requirement (see here). Nevertheless, this is not a very elegant solution, it makes reader implementations quite error-prone. If we want the alignment requirement and don't want to change the spec, it seems the most robust solution would be right now to not use an explicit padding, but instead make the writer add whitespace (or so) to the header, so that the start of the body becomes 4-byte-aligned. Does it make sense to remove that passage from the spec again? I guess it only makes sense if you want to use some Float32 arrays directly in JS... (last discussion post here) If we decide it still makes sense to have this alignment, we should provide a hint to implementers where / how to deal with that (writer side / reader side).

I think it is useful to have the 4-byte alignment (we did require it in Cesium at one point, but the code could have changed). The extension spec says that the way to get 4-byte alignment is to add trailing spaces to the JSON:

The start of body is 4-byte aligned to ease its use with JavaScript Typed Arrays. This implies that trailing spaces may be added to the JSON in the scene part such that (20 + sceneLength) is divisible by 4.

Does this work for you?

@tfili do you have any other comments?

[pjcozzi]

4 . It seems Cesium (and the writer that was used for the examples) calls the special buffer "KHR_binary_glTF", but the spec calls it just "binary_glTF". One of both should probably be adjusted.

@tfili I think we should leave the extension spec alone and update the converter and Cesium (with backwards compatibility in Cesium).

[pjcozzi]

5 . With the KHR_materials_common extension, there are no techniques / shaders that must be defined - I like that a lot :-) It enables usage across systems and shader languages. However, this requires you even more to adhere to a fixed naming of the vertex attributes, since their semantics are defined nowhere when the KHR_materials_common extension is used. In the case of our own rendering system, we used "position" and "normal", for example, whereas I had to export using "POSITION" and "NORMAL" to make the models work with Cesium. I would vote against any over-generalization to fix this issue (that would just make implementations more complex error-prone) and instead just propose to nail down a set of common attribute names for usage with KHR_materials_common. What do you think?

Although glTF uses "by semantic", not "by name", I agree that there is no place to put the semantic so I think it is fine to have well-known attribute ids just like KHR_binary_glTF has a well-known buffer id. I think that is what this part of the extension spec is trying to say:

Interaction Between Attribute Semantics and Common Materials

The base specification defines attribute semantics for mesh primitives. The common materials in this extension shall support the semantics POSITION, NORMAL, TEXCOORD, COLOR, JOINT, JOINTMATRIX, and WEIGHT. For array semantics such as texture coordinates, the implemention is only required to support the 0th set of coordinates i.e. TEXCOORD_0.

If a conforming implementation of this extension supports skinned animations, then the common materials described in this extension must also support hardware skinning in its vertex and fragment shader programs.

Can you open a pull request into the glTF repo to make it more explicit?

[pjcozzi]

6 . In parts related to the last point: it seems a bit odd that the bounding box of the model must be indirectly defined via the POSITION attribute. It would probably be very useful to have (optional) bounding box definitions for "mesh" and / or "primitive". One reason is that one could use custom shaders / attributes that encode the vertex positions differently (see WEB3D_quantized_attributes extension), and therefore have min/max values that differs from the bounding volume of the respective primitive / mesh.

This could be a nice glTF extension and perhaps even the same extension used for collision volumes. @MattMcMullan is also interested in this. Submit an issue to the glTF repo for discussion when you are ready.

[mlimper]

The extension spec says that the way to get 4-byte alignment is to add trailing spaces to the JSON:

Oh, how did I miss that? Yeah, this obviously matches with what I had in mind. So we would be able to fix this on the server side (i.e., in my converter).

[mlimper]

Let me track the sub-issues / TODOs that are part of this PR:

• 1. - definition of type/value
  • discuss
  • adjust implementation(s)
• 2. - shader parameter handling
  • discuss
  • adjust implementation(s)
• 3. - alignment of binary body
  • discuss
  • adjust implementation(s)
• 4. - adjust naming of binary buffer (Cesium or Spec)
  • discuss
  • adjust implementation(s)
• 5. - attribute semantics in spec
  • discuss
  • clarify in spec
• 6. - bbox feature
  • discuss
  • open issue
• 7. - interpretation of specular shininess parameter
  • discuss
  • adjust implementation(s)

[pjcozzi]

The extension spec says that the way to get 4-byte alignment is to add trailing spaces to the JSON:

Oh, how did I miss that? Yeah, this obviously matches with what I had in mind. So we would be able to fix this on the server side (i.e., in my converter).

Cool, sounds good!

[mlimper]

I believe the models and Cesium code are right and that the extension spec is wrong (well, not up to date). This was discussed starting with this comment: KhronosGroup/glTF#424 (comment). Do you want to open a pull request to update the extension in the glTF repo?

I see - sorry I didn't follow that discussion so closely at that point... otherwise, I would have probably supported the argumentation of @tparisi that the definition of the types should be known in advance ;-)
With a 1:1 mapping of parameter names to types, as suggested by the table from the spec, the implementation would be really trivial (see helper function).
What I understood from the comments, and the spec, however, is that mappings from parameter names to types to be 1:many. That is why there is still a "type" property in Cesium.

In fact, I got the impression that we actually have to explicitly allow only a very few different cases:
As mentioned in the comments (and in the spec), one can have diffuse / specular / ... values defined in textures... if there are only, say, these two possibilities (single value per material, or a texture), one could also use "diffuse" and "diffuseTex", and have the type of both (and the way they are interpreted) being defined in the spec. This is something we also do in X3DOM for the CommonSurfaceShader node: you can specify either a texture for a certain property (e.g., for diffuseColor, for shininess, ...), and you can specify a single value, which will act as factor (see tutorial and example). This even allows re-using the same texture with different color factors, if needed.

Two alternative suggestions:

• Update spec to allow "diffuse" or "diffuseTexture" (etc.) - everything has a pre-defined type
• Update spec to contain explicit "type" property.

After all, both of these alternatives do the same thing, which is to allow only a pre-defined set of valid types and values. However, I would prefer the first way, it seems more flexible to me (you can blend textures with a factor), but still easier/cleaner to implement.

@tfili What do you think?

[mlimper]

5.: PR to KHR_materials_common spec issued:
KhronosGroup/glTF#506

[mlimper]

4 . It seems Cesium (and the writer that was used for the examples) calls the special buffer "KHR_binary_glTF", but the spec calls it just "binary_glTF". One of both should probably be adjusted.
@tfili I think we should leave the extension spec alone and update the converter and Cesium (with backwards compatibility in Cesium).

I adapted this in Cesium as part of this PR: mlimper@948b1ed

Okay like this?

[mlimper]

This could be a nice glTF extension and perhaps even the same extension used for collision volumes. @MattMcMullan is also interested in this. Submit an issue to the glTF repo for discussion when you are ready.

Done: KhronosGroup/glTF#507

[mramato]

I don't think we want to ignore any of these files (and they are also already committed in master, so ignoring them would product odd behavior). What is your reasoning for adding them here?

[mlimper]

Thanks for the hint!
My Webstorm seemed to have changed some of them automatically, and I didn't want to commit those changes (but then I don't need to add the changes to .gitignore, of course). I wanted to revert this inside the pull request, but didn't do it so far.

[mramato]

No problem, I'm not WebStorm expert so I wanted to make sure we didn't do something non-standard on our side. Most of us our running WebStorm 11, so if you're on an earlier version, that could be the source of it changing files locally. Out of curiosity, what did it change? Maybe we can prevent that from happening with a different config tweak.

[mlimper]

I am using an older WebStorm version - that's probably the problem.

Here's a diff:

diff --git a/.idea/cesium.iml b/.idea/cesium.iml
index 9d1caef..1f7737b 100644
--- a/.idea/cesium.iml
+++ b/.idea/cesium.iml
@@ -3,6 +3,7 @@
   <component name="NewModuleRootManager">
     <content url="file://$MODULE_DIR$">
       <sourceFolder url="file://$MODULE_DIR$/Specs" isTestSource="true" />
+      <excludeFolder url="file://$MODULE_DIR$/Apps/SampleData/models/BloodhoundSSC" />
       <excludeFolder url="file://$MODULE_DIR$/Build" />
       <excludeFolder url="file://$MODULE_DIR$/Instrumented" />
     </content>
diff --git a/.idea/encodings.xml b/.idea/encodings.xml
index 97626ba..f758959 100644
--- a/.idea/encodings.xml
+++ b/.idea/encodings.xml
@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <project version="4">
-  <component name="Encoding">
+  <component name="Encoding" useUTFGuessing="true" native2AsciiForPropertiesFiles="false">
     <file url="PROJECT" charset="UTF-8" />
   </component>
 </project>
\ No newline at end of file
diff --git a/.idea/inspectionProfiles/Project_Default.xml b/.idea/inspectionProfiles/Project_Default.xml
index eff7139..76d98c8 100644
--- a/.idea/inspectionProfiles/Project_Default.xml
+++ b/.idea/inspectionProfiles/Project_Default.xml
@@ -1,6 +1,7 @@
 <component name="InspectionProjectProfileManager">
-  <profile version="1.0">
+  <profile version="1.0" is_locked="false">
     <option name="myName" value="Project Default" />
+    <option name="myLocal" value="true" />
     <inspection_tool class="JSHint" enabled="true" level="ERROR" enabled_by_default="true" />
   </profile>
 </component>
\ No newline at end of file
diff --git a/.idea/jsLinters/jshint.xml b/.idea/jsLinters/jshint.xml
index aed467c..bf9607e 100644
--- a/.idea/jsLinters/jshint.xml
+++ b/.idea/jsLinters/jshint.xml
@@ -5,31 +5,25 @@
     <option bitwise="true" />
     <option boss="false" />
     <option browser="true" />
-    <option browserify="false" />
     <option camelcase="false" />
     <option couch="false" />
     <option curly="true" />
     <option debug="false" />
     <option devel="false" />
     <option dojo="false" />
-    <option elision="false" />
-    <option enforceall="false" />
     <option eqeqeq="true" />
     <option eqnull="false" />
     <option es3="false" />
-    <option es5="false" />
     <option esnext="false" />
     <option evil="false" />
     <option expr="false" />
     <option forin="true" />
     <option freeze="false" />
     <option funcscope="false" />
-    <option futurehostile="false" />
     <option gcl="false" />
     <option globalstrict="false" />
     <option immed="false" />
     <option iterator="false" />
-    <option jasmine="false" />
     <option jquery="false" />
     <option lastsemic="false" />
     <option latedef="false" />
@@ -37,13 +31,11 @@
     <option laxcomma="false" />
     <option loopfunc="false" />
     <option maxerr="50" />
-    <option mocha="false" />
     <option mootools="false" />
     <option moz="false" />
     <option multistr="false" />
     <option newcap="false" />
     <option noarg="true" />
-    <option nocomma="false" />
     <option node="false" />
     <option noempty="true" />
     <option nomen="false" />
@@ -58,24 +50,19 @@
     <option plusplus="false" />
     <option proto="false" />
     <option prototypejs="false" />
-    <option qunit="false" />
     <option quotmark="false" />
     <option rhino="false" />
     <option scripturl="false" />
     <option shadow="false" />
-    <option shelljs="false" />
-    <option singleGroups="false" />
     <option smarttabs="false" />
     <option strict="true" />
     <option sub="false" />
     <option supernew="false" />
     <option trailing="false" />
-    <option typed="false" />
     <option undef="true" />
     <option unused="false" />
     <option validthis="false" />
     <option white="false" />
-    <option withstmt="false" />
     <option worker="false" />
     <option wsh="false" />
     <option yui="false" />
diff --git a/.idea/misc.xml b/.idea/misc.xml
index 72abef0..19f74da 100644
--- a/.idea/misc.xml
+++ b/.idea/misc.xml
@@ -10,4 +10,5 @@
     <ConfirmationsSetting value="0" id="Add" />
     <ConfirmationsSetting value="0" id="Remove" />
   </component>
+  <component name="ProjectRootManager" version="2" />
 </project>
\ No newline at end of file

[mlimper]

Sub-issue 7:
There has been a problem with the shininess parameter, as it is interpreted by Cesium / defined to be handled according to the spec. My exporter receives an X3D scene as input, where shininess is in range [0,1]. So, a value of 0.75, for example, corresponds to a high shininess, and should result in a high exponent. The spec says:
"To maximize application compatibility, it is suggested that developers use the Blinn-Torrance-Sparrow model for shininess values in the range of 0 to 1. For shininess values greater than 1.0, it is recommended to instead use the Blinn-Phong approximation: [...]"
(see here)

Implementing this would probably look somehow like this: mlimper@c566172

I think assuming a range [0, 128] or [0, 256] is pretty common, so one could clarify this in the spec and say: if the value is smaller than 1, map it to [0, 128] or so, otherwise use the value as-is. The current phrasing seems a bit complicated (at least I don't know the blinn-torrance-sparrow model).

An alternative could be to remove this part from the spec again and say that the value will be interpreted as-is, and hence exporters should now that 1 corresponds to a low shininess, and not to a high one.

[pjcozzi]

I adapted this in Cesium as part of this PR: mlimper@948b1ed

Okay like this?

No. The line we want to change is:

if (id === 'CESIUM_binary_glTF' || id === 'KHR_binary_glTF') {

It should be (include the comment):

// The 'KHR_binary_glTF' check is for backwards compatibility with Cesium 1.15.
if (id === 'CESIUM_binary_glTF' || id === 'binary_glTF' || id === 'KHR_binary_glTF') {

[pjcozzi]

Sub-issue 7:
...
I think assuming a range [0, 128] or [0, 256] is pretty common, so one could clarify this in the spec and say: if the value is smaller than 1, map it to [0, 128] or so, otherwise use the value as-is. The current phrasing seems a bit complicated (at least I don't know the blinn-torrance-sparrow model).

I don't know this spec/code well, but your solution sounds like it would work well and would simplify the logic in the client.

If @tfili agrees, perhaps open a pull request in the glTF repo.

[tfili]

I agree. I think mapping values 0-1 to [0,128] is a good solution.

[tfili]

@mlimper As far as the type situation, I think I'd prefer having an explicit type but I wouldn't be against the diffuse/diffuseTexture solution.

[mlimper]

It should be (include the comment):

Gotcha, changed that (503f087)

[pjcozzi]

@mlimper for this PR, also add yourself to CONTRIBUTORS.md.

[mlimper]

I agree. I think mapping values 0-1 to [0,128] is a good solution.

Issued a PR to get this clarified within the spec:
KhronosGroup/glTF#510

[mlimper]

@mlimper As far as the type situation, I think I'd prefer having an explicit type but I wouldn't be against the diffuse/diffuseTexture solution.

@tfili If we wouldn't have an explicit type, do you think it would be possible to check if the parameter is a string, using the typeof statement? This would only need to be done for the diffuse, specular and emission properties, as those are the only one where the type is ambiguous, other properties have a fixed type any way.

[mlimper]

Okay, I cleaned up the code a bit. It can now deal with the old/current glb files, where the parameters are objects with type/value, as well as with new ones that directly list parameter values, as suggested by the spec.

Now, there are also checks to determine whether the diffuse, emission and specular parameters are given as strings. If so, we know they have a sampler2D type. As mentioned in the previous post, an alternative would be to explicitly use diffuse/ diffuseTexture, ... parameters, but I think it's okay the way it is, since we don't have to change the spec right now.

This PR should now be ready for code review / merge.

[tfili]

This should probably use the defined function that is used throughout Cesium.

[mlimper]

@Qantas94Heavy @tfili Thanks for your helpful suggestions, I adapted the code.

[pjcozzi]

@mlimper can you please merge in master so this can be merged?

@tfili do you have any other comments or need to do any other testing?

[pjcozzi]

Also:

@mlimper can you update CHANGES.md?

@tfili what converter, doc, and test changes does this need?

[mlimper]

@mlimper can you please merge in master so this can be merged?
@mlimper can you update CHANGES.md?

Done, actually the conflict was only in CONTRIBUTORS.md.
Also added a line to the change log.

[pjcozzi]

Thanks @mlimper. @tfili this is waiting on you.

[tfili]

Looks good to me.

[pjcozzi]

Did you see my questions:

@tfili what converter, doc, and test changes does this need?

[tfili]

Missed that one. One thing I noticed is that this change uses double quotes and the rest of Cesium uses single quotes.

As for the questions:

For Converter
We need remove the exporting of type as we now figure that out at runtime. Added issue here.

For Doc
Shouldn't need to change as the spec is updated correctly.

For Test
We'll want to update the test models & sand castle models to use the new format. These can't be updated until the converter changes so we can probably take this change and update the models in another PR.

[pjcozzi]

@mlimper one last request: can you change this PR (or open a new one) into the new gltf-materials branch so @tfili and I can update the test models?

Thanks again for all your work on this!

[mlimper]

@mlimper one last request: can you change this PR (or open a new one) into the new gltf-materials branch so @tfili and I can update the test models?

I have opened a new one here:
#3410

Thanks again for all your work on this!

I'm happy to have the chance to contribute to this great project.