[x3d-public] Various comments reading X3Dv4 current working draft (WD2 in GitHub repository)
Don Brutzman
brutzman at nps.edu
Thu Dec 3 10:51:53 PST 2020
Apologies for delay, had some system difficulties. Here are review results from Dick and I today.
On 11/29/2020 1:02 PM, Michalis Kamburelis wrote:
>
> 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...."""
Correction applied.
> 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.
Agreed, applied slight rephrase:
The <i>shadowIntensity</i> field has no effect when the <i>shadows</i> field is FALSE.
> 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]."
> 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.
OK
> I moved some of this text to my wiki,
> https://github.com/michaliskambi/x3d-tests/wiki/Make-RGB-and-grayscale-textures-treatment-consistent
> .
Good thinking, thank you
> 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."""
Agreed, rephrased as
"The castShadow field defines whether this Shape casts shadows as produced by lighting nodes. If the visible field is FALSE, then the Shape does not cast any shadows, regardless of the castShadow 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.
Our changes above are checked in. Please make these detailed equation-related changes yourself and commit.
There will still be some forthcoming changes to references and English phrasing, please update before modifying, then let us know when complete.
> 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)
> """
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.
> 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...
> Regards,
> Michalis
Thanks, let's keep going...
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
More information about the x3d-public
mailing list