[x3d-public] Various comments reading X3Dv4 current working draft (WD3 in GitHub repository)
Don Brutzman
brutzman at nps.edu
Sat Dec 5 19:42:50 PST 2020
On 12/5/2020 4:33 PM, Michalis Kamburelis wrote:
>
> Done!
>
> I made the modifications listed in this thread (see points 3,6,8,9 above).
Great, mercifully it all seemed to merge just fine with my local gitstore.
> I also fixed a new error I found in "17.5 Support levels". Someone
> wrote there "Support for Physical and Unlit shading." -- which is not
> correct. As we discussed in an earlier thread, and I outlined in
> https://github.com/michaliskambi/x3d-tests/wiki/Do-not-confuse-Phong-shading-with-Phong-lighting-model
> , there's no such thing as "physical shading" or "unlit shading". This
> is confusing "shading" with "lighting model". So I simply changed it
> to "Support for Physical and Unlit lighting models.".
Yes that was our intent, thanks for fixing that misstep.
> Checked with "ant" that I didn't introduce any new errors/warnings.
hat's off to you, thanks! confirmed.
> My commits are on
> https://github.com/Web3DConsortium/X3D/commit/4021f559e6498cc7807700dc1aef416bea01000f
> so you can see exactly what I changed.
>
> Regards,
> Michalis
better and better, changes look good from here tonight, as already discussed Dick and I will continue review of prose in Lighting Shape and related components.
> niedz., 6 gru 2020 o 00:29 Michalis Kamburelis
> <michalis.kambi at gmail.com> napisał(a):
>>
>> I'm sitting down to do the changes outlined in this thread now :) So I
>> will make a commit+push soon, will let you know (this should not take
>> me more than ~1 hour).
>>
>> Regards,
>> Michalis
>>
>> pt., 4 gru 2020 o 02:12 Don Brutzman <brutzman at nps.edu> napisał(a):
>>>
>>> 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
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