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

[Michalis Kamburelis]

Hi,

As the X3Dv4 deadline is looming, I scanned the last WD2 version (from
the GitHub repository, so it is the latest "master" -- not what was
published as "WD2" on the web3d website). I focused on things in which
I was somehow involved -- PBR, new lighting models, shadows, PTM.

I list below a list of things that are important to improve, from my
perspective. I can do most of them myself (1, 2, 3, 5, 6, 8, 9) --
just tell me "go ahead and do it Michalis", and I will commit+push
them to X3D spec master. I hope this will take off some work from you.
Don, Richard -- I know you have your hands full finalizing everything,
thank you for keeping this going.

1. I see you added "17.2.2.8 Shadows". Looks generally good.

    Correction from me to the first sentence:

    """"Lighted geometric objects can cast shadows, meaning that...."""

    ->

    """Geometric objects can cast shadows, meaning that...."""


    That is, I would remove the "lighted" word. Because it is
confusing, and not necessary. Unlit geometric objects can (and should)
actually cast shadows just like lit geometry objects. The text in
X3DLightNode, later, talks about "illuminated X3DShapeNode geometry"
and that is correct and more precise.

2. In "17.3.1 X3DLightNode":

    I would heavily advice to add one more sentence to the paragraph
about "shadowIntensity":

    "The <i>shadowIntensity</i> only matters when the <i>shadows</i>
field is TRUE."

    This makes our intention 100% clear.

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

4. I see you simplified some text in "17.2.2.2 Texture sampling", to
which I agree. I put there some notes about compatibility and
optimization, which are probably not within the scope of X3D 4
standard.

    I moved some of this text to my wiki,
https://github.com/michaliskambi/x3d-tests/wiki/Make-RGB-and-grayscale-textures-treatment-consistent
.

5. In "12.3.5 X3DShapeNode"

    I would heavily advise to explicitly clarify what is more
"important", the "visible" or "castShadow" field. Because for
implementations, casting shadows from invisible shapes is possible
(yet, it is not something we want). The correction is simple:

    """If the <i>visible</i> field is FALSE, then the Shape does not
cast any shadows."""

    ->

    """If the <i>visible</i> field is FALSE, then the Shape does not
cast any shadows, regardless of the <i>castShadow</i> value."""

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.

7. "42  Texture projector component" I see you already have work
in-progress to rephrase the projective texturing to be a light source,
OK.

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

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

Regards,
Michalis