[x3d-public] Various comments reading X3Dv4 current working draft (WD2 in GitHub repository)

[Michalis Kamburelis]

Thank you Don! Comments (only to 3,6,8,9) below:

> > 3. In "17.2.2.6 Physical lighting model"
> >
> >      I see you removed the last paragraph, that refers to glTF lighting
> > equations and was saying "Future revisions of this draft will contain
> > the final recommended equations." Of course that is OK, we don't want
> > to leave a text saying "Future revisions of this draft...." in a final
> > specification :)
> >
> >      And it is my fault, I promised but completely failed to create a
> > correct prose for this text with equations. But we need to say
> > *something* about what should be done. I propose this text:
> >
> >      """Using the <i>baseParameter</i>, <i>metallicParameter</i> and
> > <i>roughnessParameter</i>, the physical lighting model calculates the
> > <i>physicalLightContribution<sub>i</sub></i> value. The calculation
> > should follow the standard Physically Based Rendering equations, and
> > be consistent with the glTF 2.0 lighting model.
> >      """
>
> Rephrased:
>
> "The physical lighting model uses the baseParameter, metallicParameter and roughnessParameter to calculate the physicalLightContribution_i value in accordance with the physically based rendering equations as specified in 2.[GLTF]. The ambientIntensity value is unused."
>
> Edited second sentence, placed it later under physical lighting mode, as
>
> "The physical lighting equations are specified in 2.[GLTF]."

Your rephrasing is perfectly OK, thank you.

However, you added it to "17.2.1.1 Overview" instead of at the end of
"17.2.2.6 Physical lighting model". This makes the paragraph
confusing, as it talks about baseParameter, metallicParameter
roughnessParameter, physicalLightContribution_i --- all of them are
only defined in "17.2.2.6 Physical lighting model".

I would suggest to move this text to "17.2.2.6 Physical lighting
model", at the end.

> > 6. The lighting equations have not been updated to reflect shadows. I
> > provided a prose for it, on 2020-10-20, repasting it below:
> >
> >      """
> >      shadowTest_i scales down the light contribution depending on shadow:
> >
> >      - shadowTest_i is 1.0 if the light source i has "shadow" = FALSE
> >
> >      - otherwise, shadowTest_i is 1.0 if the light source i has
> > "shadow" = TRUE but nothing obscures the light, i.e. there is no Shape
> > (that has visible and castShadow = TRUE) between the light source
> > position and the given point. The DirectionalLight is treated like a
> > "point in infinity", which means that shadow rays are all parallel to
> > the light direction.
> >
> >      - otherwise, shadowTest_i is "1.0 - shadowIntensity". This occurs
> > when "shadow" = TRUE and the light is obscured. For example,
> > "shadowIntensity" equal 1.0 (the default) means that light
> > contribution drops to zero when the light is obscured by the shadow
> > caster.
> >      """
> >
> >      And replace the "on_i" with "on_i * shadowTest_i" in above
> > equations, to use shadowTest_i to scale down the light contribution.
>
> Our changes above are checked in.  Please make these detailed equation-related changes yourself and commit.

OK, I'll do it (late Saturday, most likely, is when I'll find the
earliest possible time).

Please don't wait for me -- whatever you do in the meantime, I'll
manage to resolve and commit my changes.

> > 8. "9.5 Support levels" (in Networking component)
> >
> >      About the text mentioning glTF at the end, "Support for .gltf
> > (model/gltf+json) and .bin (application/octet-stream). Requires
> > support for Shape component level 2 and Lighting component level 4."
> >
> >      I see you didn't apply there my notes from 2020-08-09 (thread "Re:
> > X3D minutes16 July 2020: Inline refresh, https, description;
> > X3DUrlObject: glTF support") :) The text most likely doesn't say what
> > you want, because X3D specification isn't really concerned about .bin
> > files (they are not referenced from X3D, they are only referenced from
> > glTF), and the text doesn't mention .glb (and it probably should). I
> > propose to change it to this:
> >
> >      """
> >      Support for glTF models in Inline nodes, with .gltf
> > (model/gltf+json) and .glb (model/gltf-binary) formats.
> >
> >      Minimum required glTF support:
> >      - glTF transformation hierarchy,
> >      - glTF meshes,
> >      - glTF standard physical materials (correspond to X3D
> > PhysicalMaterial nodes, requires "Shape" component level 2 support)
> >      - glTF unlit (KHR_materials_unlit) materials (correspond to X3D
> > UnlitMaterial nodes)
> >      - loading of external binary data referenced from .gltf files
> > (e.g. for vertex coordinates)
> >      """
>
> Wondering, what is left out from your list?
>
> We don't have to exclude glTF extensions, and we don't have to omit things that implementers might optionally include.
>
> Am thinking that, if subsetting is actually needed, this goes better in the appropriate support table.  Seems unnecessary.

This lists the "Minimum required glTF support:", so it's not about
excluding, but including :)

That is, I want to specify a *required subset of glTF* for an X3D
browser to claim "I support X3D Networking component at level 4"
(which means, glTF support).

The big things missing (that is, *not* required from X3D browser, with
my proposed text) are

- skinned animation in glTF
- morph target animation in glTF

I can go in details, but basically we're still figuring out (at
implementations, like CGE or X3DOM) how to express these things in X3D
without losing any information or efficiency. While CGE supports glTF
skinned animation, my solution is not final, I'd like to use some
standard X3D nodes for that in a standard way -- but no implementation
has yet figured out how to do it perfectly (see my
https://github.com/michaliskambi/x3d-tests/wiki/Converting-glTF-to-X3D
).

There is also lots of small things, for which CGE and/or X3DOM have
extensions, but they are not yet in X3D standard (alphaMode,
alphaCutoff, per-vertex colors with multiplication, inverting vertical
texture coords orientation, details about punctual lights etc.).

So we're not yet at level "you can express 100% of glTF using standard
X3D 4.0 nodes". We got closer to it with PBR, but there's still work
ahead :)

>
> > 9. " 12.2.4 Texture mapping specified in material nodes"
> >
> >       I promised to provide a prose making the soft-fail (I believe
> > both Richard and Andreas said it's a good idea), to specify what
> > happens if given "xxxTextureMapping" is not existing or existing
> > multiple times in the relevant list. Here's are 2 corrections, please
> > apply:
> >
> >       In "12.2.4.1 Texture coordinates" :
> >
> >       """If the xxxTextureMapping field is not empty, it must refer to
> > a corresponding X3DSingleTextureCoordinateNode node on a list of
> > texture coordinates."""
> >
> >       ->
> >
> >       """If the xxxTextureMapping field is not empty, it refers to a
> > corresponding X3DSingleTextureCoordinateNode node on a list of texture
> > coordinates. If no corresponding X3DSingleTextureCoordinateNode is
> > found, the browser should determine texture coordinates as if
> > xxxTextureMapping was empty (see below). If multiple
> > X3DSingleTextureCoordinateNode nodes with the same "mapping" value are
> > present, the first one is chosen."""
> >
> >       In " 12.2.4.2 Texture coordinates transformation" :
> >
> >       """If the xxxTextureMapping field is not empty, it must refer to
> > a corresponding X3DSingleTextureTransformNode node within the list of
> > texture transformations. The X3DSingleTextureTransformNode node must
> > have equal mapping value."""
> >
> >       ->
> >
> >       """If the xxxTextureMapping field is not empty, it refers to a
> > corresponding X3DSingleTextureTransformNode node within the list of
> > texture transformations. The X3DSingleTextureTransformNode node must
> > have equal mapping value. If no corresponding
> > X3DSingleTextureTransformNode is found, the browser should determine
> > texture transformation as if xxxTextureMapping was empty (see below).
> > If multiple X3DSingleTextureTransformNode nodes with the same
> > "mapping" value are present, the first one is chosen."""
>
> Please make the changes you think best and then we will review.
>
> We are still thinking about a better way to express xxxTexture and xxxTextureMapping as a way to refer to similarly named fields.  Seems cheesy to me...
>

OK. I admit I don't like the"xxxTextureMapping" / "xxxTexture" phrases
myself, but it was the best I could come up with that is simple and
unambiguous to the reader :)

In the meantime I would suggest to apply my functional change above,
that introduces the "soft-fail" mechanism that Richard and Andreas
requested. Let's have at least functionality perfect :)

Regards,
Michalis