[x3d-public] Specification editors mantis issues: PBR review

[Michalis Kamburelis]

Hi!

Sorry again that I could not attend our scheduled meeting today. We'll
coordinate a new meeting, and in the meantime I'll try to address the
questions by email, so that the public mailing list has all the info :)

Answers are inline below.

Don Brutzman <brutzman at nps.edu> wrote:

> ---
>
> 2. Spec questions.
>
> a. see new inquiries today regarding PointProperties from Andreas and
> Holger.  Let's continue/contribute on that thread please.
>

I do not see input from Holger in the PointProperties thread. If there's
some communication off-list, maybe because someone forgot to use "Reply
All", please resend it to the mailing list :)


>
> ---
>
> b. 12.2.2 Appearance characteristics
>
> Seems like we can remove the sentences starting with "This set of nodes
> may be extended by creating new nodes derived from..."
>

Sure, yes.


>
> If so, we can likely merge the two corresponding bullet lists.
>
> We might also remove the sentence "This set may be extended by creating
> new nodes derived from the abstract X3DTextureNode base class as defined in
> 18.3.2 X3DTextureNode."
>
> We don't have to give guidance about extending.  (Prototypes are based on
> the first node within ProtoBody so those rules are separately defined.)
>

Yes, agreed.


>
> ---
>
> c. 12.2.3 Two-sided materials
>
> - small wording changes in first section.
>
> - Can't a model Appearance without a material field still have a
> backMaterial field?
>

No, because that would not be clear.

Using "backMaterial" means that you want back faces to have different
colors than front faces. Using "backMaterial" without "material" would mean
that you didn't specify what are the front colors. If we would allow this,
we would have to make a decision between two sub-optimal choices:

1. "backMaterial <> NULL" and "material = NULL" => means that front is
white unlit, while back is "as specified". This is a sub-optimal choice,
because it is inconsistent with "backMaterial = NULL" and "material <>
NULL", in which both front and back materials are "as specified".

2. "backMaterial <> NULL" and "material = NULL" => means that front is just
like back. This is a sub-optimal choice, because user could achieve the
same effect by moving the node from "backMaterial" to "material", and then
it would be also compatible with browsers that don't implement
"backMaterial" (e.g. because they don't yet support all X3D v4 features, or
they don't plan to implement 2-sided materials with different colors). So
authors should place the material node in "material" field to get this
behaviour.

So it seems like the optimal choice is to prohibit this. That is,
"backMaterial <> NULL" is only allowed when "material <> NULL".


>
> - If Appearance material has TwoSidedMaterial and backMaterial has another
> node, can't backMaterial take precedence?
>

No, IMO.

That would mean we ignore part of the information ("material" value), and
also that we don't know what is "front" color. If anything,
TwoSidedMaterial should take precedence, because TwoSidedMaterial has full
information. But then, TwoSidedMaterial is deprecated, so it doesn't feel
right that it takes precedence.

And this means we have a decision with no good solution :)

So it is better to simply prohibit specifying two things that conflict with
each other. Authors should not use TwoSidedMaterial and backMaterial at the
same time. Conversion from TwoSidedMaterial to backMaterial approach is
straightforward, and can be automated. We should encourage authors to make
this conversion :)


> - Appearance only has one texture field.  Why are you talking about
> DEF/USE?
>

Because textures are also inside material nodes. They can be DEF/USEd
between front and back material. The prose should make it clear:


"""
Use the same textures with the same mapping.
In other words, the values of the fields <i>xxxTexture</i>
and <i>xxxTextureMapping</i> documented in
<a href="#TextureMapping">12.2.4 Texture mapping specified in material nodes
</a>
shall be equal for both front and back materials.
"""


>
> We need to discuss the various cases, seems like the allowed combinations
> might be stated more simply.
>
> I suspect that if we primarily define material and backMaterial, then note
> how TwoSidedMaterial can be used, that best sets us up for long term (when
> TwoSidedMaterial is no longer supported).
>

Agreed, we indeed should focus on (new, non-deprecated) backMaterial
behaviour prose.

The TwoSidedMaterial can then be explained like "if you use
material=TwoSidedMaterial, then it is equivalent to using material=Material
and backMaterial=Material like this: ...." and here we can write a prose
and an example.


>
> ---
>
> 12.2.4 Texture mapping specified in material nodes
>
> - This section doesn't make much sense, especially the "xxxTexture"
> phrasing (I think you mean "emissiveTexture and normalTexture" in
> X3DOneSidedMaterialNode).  Let's discuss please.
>
> Striving for simplification and clarity as best way to achieve correctness.
>

It is an extremely important section :) And by "xxxTexture" I mean **all
"xxxTexture" fields in all XxxMaterial nodes**. So

- emissiveTexture,
- normalTexture,
- diffuseTexture,
- specularTexture,
- ambientTexture,
- baseTexture,
- metallicRoughnessTexture,
- occlusionTexture.

Yes, there are a lot of them. That's how modern materials work (in glTF,
Blender, Unity, everywhere?). *All* material parameters can be adjusted by
some texture.


>
> ---
>
> 12.2.5 Coexistence of textures specified in material nodes with the
> "Appearance.texture" field
>
> This might be greatly simplified and merged if we get prior
> simplifications done well.
>
> ---
>
> 12.3.4 X3DOneSidedMaterialNode
>
> - Your "Editorial note: We consider merging this abstract node with
> X3DMaterialNode."
>
> Am thinking we should keep both of these in order to support Material
> (which won't change) and X3D3 versus X3D4 PBR rendering.
>

Supporting Material has no relation to this decision. Of course we will
support Material (with Phong lighting model, 100% compatible with X3D 3).

But this is not related to the decision "do we merge X3DMaterialNode with
X3DOneSidedMaterialNode". This decision is dictated by the need to support
TwoSidedMaterial for the time being.

I'm also after *not* merging (IOW, not changing the current spec), my
reasons are on
https://github.com/michaliskambi/x3d-tests/wiki/How-to-add-PBR-to-X3D%3F#why-do-we-need-new-x3donesidedmaterialnode-cannot-we-add-fields-like-emissivexxx-to-x3dmaterialnode
.



>
> - "tangent space" discussion can be simplified.
>

Agreed, definitely. I got carried away when writing this :) The "tangent
space" term, especially when used in the context of normalmaps, is probably
rather clear to everyone, without my long and clumsy explanations in the
spec. After my sentence

   """The normals are provided in the <i>tangent space</i>"""

....I wrote a lot of text/examples that can be definitely
removed/simplified.  Looking forward to simplifying this together :)


>
> ---
>
> 12.4.5 Material
>
> - You have a number of textures (seven total!) but only two are listed in
> X3DOneSidedMaterialNode.  Shouldn't they be in the abstract node type also?
>

No.

The very idea of the abstract type "X3DOneSidedMaterialNode" is that it
serves as the ancestor for various other materials: Material,
PhysicalMaterial, UnlitMaterial.

The "X3DOneSidedMaterialNode" is *not* a duplication of "Material", that
would not be useful. The "X3DOneSidedMaterialNode" is an ancestor in OOP
terminology, just like all other abstract nodes throughout X3D
specification.

Feel free to paste this answer to Mantis too. We'll talk about this at our
next meeting. Thank you for review, looking forward to finishing this
together :)

Best regards,
Michalis
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://web3d.org/pipermail/x3d-public_web3d.org/attachments/20200824/0cba1c24/attachment.html>