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

[Don Brutzman]

All good.  Have corrected subject line to WD3.

Please make the changes you think are appropriate for all of these points.

Recommend "simple is good" regarding glTF level of support.  If it is not changing component/level requirements, if it avoids  need for minor changes over the next year, that is fine.  Active implementations always have gaps (and steady increases) in capabilities.

Lighting shading textures rendering - all yours, again.  8)

On 12/3/2020 1:20 PM, Michalis Kamburelis wrote:
> 
> 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
> 

all the best, Don
-- 
Don Brutzman  Naval Postgraduate School, Code USW/Br       brutzman at nps.edu
Watkins 270,  MOVES Institute, Monterey CA 93943-5000 USA   +1.831.656.2149
X3D graphics, virtual worlds, navy robotics http://faculty.nps.edu/brutzman