Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An appearance with a diffuseColor and an RGB texture should ignore the diffuseColor #50

Open
Sgeo opened this issue Sep 29, 2019 · 27 comments
Labels
bug Something isn't working question Further information is requested wontfix This will not be worked on

Comments

@Sgeo
Copy link

Sgeo commented Sep 29, 2019

Describe the bug
When an Appearance has both a Material with a diffuseColor and an ImageTexture with RGB(A) components, the diffuseColor should be ignored, as IDRGB is not referenced when an RGB texture is in use. It currently is not.

Screenshots
The below screenshots show a globe with an RGB texture and a diffuseColor of 1 0 0. As far as I can tell, InstantReality is correct and X_ITE is incorrect

InstantReality player

image

X_ITE

image

Additional context
#38 has some discussion about this issue, however, it looks like there is a misreading of the spec as in the "Lighting 'on'" table, IDRGB, the diffuseColor, is not used for that scenario. The English text above the table seems to incorrectly describe the effect of the table, unless I am misunderstanding the table.
The VRML spec's English description seems to agree with the table, and not with the English description in the X3D spec.

Attached are files I used for testing. The relevant files are HelloWorld and HelloWorldXITE (the other files are because I was testing a differing implementation).

DiffuseColor.zip

@Sgeo Sgeo added the bug Something isn't working label Sep 29, 2019
@create3000
Copy link
Owner

This is more likely a feature than a bug. You can colorize the texture with a specific color or change the lightness of the texture when setting the diffuseColor to specific values. To get specification conform lighting calculations set the diffuseColor to 1 1 1. Various effects can be achieved with this behavior, for instance to animate the diffuseColor from white to black to fade out the texture.

The transparency of a RGB/A texture can be changed as well, this enables X3D authors to animate the transparency of a textured object to for instance create a fade in/out effect from opaque to transparent and vice versa.

On the other hand, X_ITE uses shaders for material/lighting calculation, and the default shader is a Gouraud shader where the material/lighting calculations are in the vertex shader, and the texture is applied in the fragment shader. It is technically not possible to use the texture color as diffuseColor in the vertex shader, because texture color is first available in the fragment shader. This means that the Gouraud shader is not fully specification conform. Specification conform lighting calculation is first possible in the Phong shader, which can be selected using Browser.setBrowserOption("Shading", "PHONG") in a Script node.

@Sgeo
Copy link
Author

Sgeo commented Sep 29, 2019

Am I understanding correctly that X_ITE's behavior is an intentional deviation from the spec? I wonder if there could be an option or some optional code to enable closer conformance to the spec for being able to use pre-existing assets more smoothly (although I understand there might be performance problems determining how many channels an image actually has), or at least some documentation on intentional deviations from spec.

@jamesleesaunders
Copy link

Joining conversations up:
x3dom/x3dom#993
The same discussion is being had on x3dom.

@create3000
Copy link
Owner

It is technically not possible and as it turns out it gives the X3D author more possibilities to control the color of the texture. Indeed it should be documented, updated documentation for diffuseColor and transparency, see http://create3000.de/users-guide/components/shape/material/#fields, and http://create3000.de/users-guide/components/shape/twosidedmaterial/#fields.

@splace
Copy link

splace commented Sep 30, 2019

Specification conform lighting calculation is first possible in the Phong shader, which can be selected using Browser.setBrowserOption("Shading", "PHONG") in a Script node.

are you saying the Phong renderer is spec. complaint? or that it could be? (for this and using X-ITE's current engine.)

if it is, then it should be the default, x-browser differences just drive away users.

@create3000
Copy link
Owner

Phong shader cannot be the default, spec says Gouraud is. The author must select Phong shader manually. As long as the diffuseColor is (1 1 1) there is no difference to the spec, think this should be no problem.

@Sgeo
Copy link
Author

Sgeo commented Sep 30, 2019

The X3DOM thread has a discussion about why strictly following the spec is not practical: There isn't a way to determine how many channels an image has without reprocessing it directly. I'm not entirely sure how Phong vs Gourand fits into the discussion though.

There are a number of assets (especially Cybertown assets) that have non-(1 1 1) diffuseColors with RGB textures. I assume that not much care was placed into writing such a diffuseColor, because it wouldn't have been noticeable in a spec-compliant viewer. Such assets look wrong in a non-compliant viewer that always multiplies by diffuseColor, unless the asset is changed to use diffuseColor 1 1 1 everywhere that there is an RGB texture. Someone unfamiliar with these discussions may not understand why the asset looks wrong.

@Sgeo
Copy link
Author

Sgeo commented Oct 2, 2019

This comment links to prior discussion in mailing lists about this issue. I'm not entirely sure I understand the resolution:

create3000/cobweb#34 (comment)

@michaliskambi
Copy link

From Castle Game Engine / view3dscene ( castle-engine/castle-model-viewer#21 ): We also always multiply diffuse color with the texture, regardless if the texture is RGB or grayscale. We knowingly ignore the text of the specification that says to behave differently for RGB or grayscale textures.

My reasons are now listed in castle-engine/castle-model-viewer#21 , https://github.com/michaliskambi/x3d-tests/wiki/Make-RGB-and-grayscale-textures-treatment-consistent and https://castle-engine.io/x3d_multi_texturing.php#section_default_texture_mode .

I'm working to make this behavior simpler in X3D 4. The reasons and text are on: https://github.com/michaliskambi/x3d-tests/wiki/Make-RGB-and-grayscale-textures-treatment-consistent .

@splace
Copy link

splace commented Oct 2, 2019

I'm working to make this behavior simpler in X3D 4. The reasons and text are on: https://github.com/michaliskambi/x3d-tests/wiki/Make-RGB-and-grayscale-textures-treatment-consistent .

as a counter point to the positive reasons to change this in the spec (also why i think it is this way).....

multiplying colours has no, real, physical meaning, so i would say how you do it has no fundamental definition, in the long run there may be unforeseen benefits to NOT doing it.

for example;

you could (and i think in a meaningful way) multiply the diffuseColor by the intensity (greyscale) of the texture.

@michaliskambi
Copy link

@splace
Multiplying component-wise RGB texture by RGB color seems to be standard in computer graphics, also outside X3D. See e.g. glTF material (baseColor texture multiplied component-wise by factor), Blender standard material, Unity standard material.

That's probably because it's trivial to understand and implement, and it gives author good flexibility. This way if you use a non-grayscale (e.g. yellow) diffuseColor, or non-grayscale texture, they will be used (instead of ignoring diffuseColor in some cases, if following X3D 3 spec precisely).

We can speculate why it was specified in VRML 2 this way (X3D spec inherited this part from VRML 2, as far as I recall). My guess is that this was a misguided attempt at optimization, in the old days someone was afraid that component-wise RGB multiplication at each texture sampling could have a negative effect on speed. (It has no measurable effect on speed, on existing GPUs). But I am just guessing here, I didn't receive an explanation about it on x3d-public mailing list either.

@michaliskambi
Copy link

P.S. Component-wise multiplication is also consistent with how we mix light RGB color with final (maybe coming from a mix of vectors or textures) surface RGB color. And this has a physical explanation, i.e. it is some approximation of what happens in reality (approximating light and surface color spectrum using 3 float values).

So it seems consistent that we mix material with texture the same way, unless we have good reasons to do otherwise:)

@splace
Copy link

splace commented Oct 4, 2019

Component-wise multiplication is also consistent with how we mix light RGB color with final (maybe coming from a mix of vectors or textures) surface RGB color. And this has a physical explanation, i.e. it is some approximation of what happens in reality (approximating light and surface color spectrum using 3 float values).

after some thought, yes, but maybe a way of clearly thinking/writing about it might be;

'diffuseColor' and the colour from the texture, being 3-component adjustment factors (in the range 0-1) which can sensibly be, chain, multiplied.

so 'diffuseColor' (or 'Color' node values etc.) are somewhat like paint/pigment colour, they are adjusters, but an 'emissiveColor' (or a lights color field) ARE like a 'real' light colours and shouldn't be multiplied by each other. (and aren't AFAIK)

@splace
Copy link

splace commented Oct 4, 2019

as for changing spec....

it does seem to me the existing state is simple from a world-builders point-of-view. but the bad support renders this simplicity irrelevant.

as for an existing issue...

with the current spec. as written; a world-builder might use a tool to convert to greyscale that leaves the three channels, just makes them the same, so it would be very confusing. also i read somewhere one browser checks for all grey colours and then applies 'diffuseColor' making worlds for it very unlikly to work correctly anywhere else, but still it wouldn't be against the spec!!

@splace
Copy link

splace commented Oct 4, 2019

changes to spec. an idea...

IMHO ideally the original spec. should have stuck to information obtainable from the response header only (mime-type) as x-ite/JS html browser, (both jpeg and png suffer here.) so..

does the spec. tell you how to determine grayscale-ity? so would a 'fix' be the current spec being clarified to say how its determined. like having all textures needing to be explicitly mime-typed to tell you. (image/x-portable-graymap etc.)

i think, this would actually work as new proposal. but break existing content, which is unavoidable anyway.

however, the grayscale only types aren't required to be supported, and server-side support, for getting the correct type in the header, can't be relied on.

@michaliskambi
Copy link

@splace
As for detecting the "grayscale-ity":

  • The X3D 3 specification seems to say to use image header to determine whether it's grayscale: https://www.web3d.org/documents/specifications/19775-1/V3.3/Part01/components/texturing.html#TextureMapImageFormats . Which may be uncomfortable or impossible, depending on your platform (web or not) and/or image-reading library.

  • Note that the MIME type, which you mention, is not a solution to distinguish between grayscale and RGB textures. PNG is just image/png, regardless if it's grayscale or RGB. Similar for JPEG.

    And on desktop, you either get MIME type by guessing from file extension, or by analyzing file contents. So relying on MIME doesn't introduce a new information, on desktop.

    MIME (or looking at file extension) would only work for PPM family of formats (image/x-portable-graymap for grayscale, image/x-portable-pixmap for RGB). Even there it's unreliable, as it's also valid to use image/x-portable-anymap. Apache could just return image/x-portable-anymap for all .pgm and .ppm images, and then we're back to the question "should I treat it as grayscale or RGB".

  • The nice thing about solving it by "let's do component-wise multiplication in case of RGB texture" is that this whole problem disappears. It doesn't matter if browser treats your pixels as grayscale or RGB (with all channels equal), the result is the same.

As for the world builders: I do not agree that the current X3D 3 prose (even if it would be implemented consistently by browsers) is a good mechanism for X3D authors.

  • It's contrary to what every other software is doing -- I mentioned earlier examples: glTF material, Blender standard material, Unity standard material. They all multiply component-wise RGB texture with RGB color. And this way they do not "ignore" an information from user. User can specify diffuseColor="1 1 1" if no multiplication is necessary.

  • For example, consider X3D files exported by Blender. Blender assumes that X3D does component-wise multiplication of diffuseColor with texture. So if you implement X3D browser to follow exactly X3D 3 specification (and ignore the diffuseColor in case of RGB texture), Blender users will be unhappy. It's not even possible to fix Blender exporter for this (since there's no field in X3D Material to say "multiply texture * diffuseColor, even if the texture is RGB and you implement X3D 3 specification precisely").

So, I'm still convinced that in X3D 4, we should just multiply component-wise diffuseColor by a texture. Let's follow everyone else. This makes life easier for both browser authors and X3D files authors.

@splace
Copy link

splace commented Oct 4, 2019

@michaliskambi

the 'idea' i mention is fundamentally agreeing with you, to be clear;

if you assume the spec is saying 'use the mime' then you can legitimately treat ALL image/jpeg||png as RGB like your proposal (you regard the greyscale versions as internal format compression.), just saying to 'tidy up' the spec to make this clear.

this avoids a step change in the spec. and having to support BOTH ways.

the other preexisting feature covered by the current spec. is then relocated to other mime types like PPM, which i can't see being used (for actual format isn't required) but old content relying on this feature could then be fairly easily converted.

@splace
Copy link

splace commented Oct 4, 2019

it seems that X3DOM has gone ahead and implemented sniffing x3dom/x3dom#999

@Sgeo
Copy link
Author

Sgeo commented Oct 4, 2019

At some point I have to admit that there are competing interests:

  • Always multiplying by diffuseColor is simpler to implement and is more flexible
  • Following the current spec makes historical assets work better with less effort (especially assets designed with Blaxxun 4.4 in mind, there's quite a few of those)

The thing about historical assets though is they often need to be fixed up for other reasons. and don't even work properly in descendants of the browsers they were intended for. E.g. assets that animate viewpoints... Blaxxun 4.4 didn't apply gravity during those animations, newer versions and X_ITE apparently do (is this in the spec somewhere?). I've seen an asset with a PROTO that outputs multiple nodes, is that even valid? etc. etc. That, and EXTERNPROTOs to URLs that no longer exist.

@splace
Copy link

splace commented Oct 4, 2019

@Sgeo

Blaxxun 4.4 didn't apply gravity during those animations, newer versions and X_ITE apparently do (is this in the spec somewhere?).

(this might the same thing?) i recently encountered a problem breaking some worlds, "do you fall when there isn't ground below you?" traditionally you didn't, but unfortunately its not specified, so X-ITE does so.

please let me know if it is/isn't the same.

i have been keep notes of these things, that is; old browser non-compliance, the idea to help people quickly fix old content, or at least point them in the right direction.

@splace
Copy link

splace commented Oct 5, 2019

@Sgeo

I've seen an asset with a PROTO that outputs multiple nodes, is that even valid? etc. etc.

this might have been Blaxxun.

well, as i understand it, PROTO in one node, but it can be a grouping node, and reference non display-graph nodes like Script after the 'main' node and also containing routes, at the base level.

unfortunately VRML etc. has a strange way to deal with non-conformity, it says the result is undefined, so anything, including appearing to work, is fine! never really thought this was sensible, fail fast (and hard) is better IMO.

also, i seem to remember, a browser that did not display other display-graph nodes than the first, but did allow referencing them, this isn't that clear as to if its allowed, or not, as i remember.

@brutzman
Copy link

Question: is this driving to closure satisfactorily? Are precise changes expected for X3Dv4?

The X3D Architecture Specification must have consistent rendering well defined. This is critical path forward, the X3D Working Group will refine or amend the X3Dv4 specification as needed to ensure consistent results. This is a primary feature of X3D, especially for archival publication and sharing of models.

Getting X3D Appearance of materials and texture unambiguously defined and consistently implemented is very important, especially in preparation for addition of Physically Based Rendering (PBR) and correspondingly advanced lighting model to X3Dv4.

Thanks in advance for all scrutiny and progress.

@splace
Copy link

splace commented Oct 18, 2019

Question: is this driving to closure satisfactorily? Are precise changes expected for X3Dv4?

seems to me to be fine as is, OK in html its not trivial to access the information required. no way a spec issue though.

@create3000 create3000 added the wontfix This will not be worked on label Jun 2, 2023
@create3000
Copy link
Owner

X3D 4.0 addresses this issue, see https://www.web3d.org/documents/specifications/19775-1/V4.0/index.html

diffuseParameter = mixTexture(applyColorPerVertex(diffuseParameter), diffuseTextureParameter)

So I will close this issue now.

@brutzman
Copy link

brutzman commented Jun 2, 2023

Please note that Lighting and other rendering-related components underwent some reorganization as part of X3D 4.0, current link to Lighting component follows.

@brutzman
Copy link

brutzman commented Jun 2, 2023 via email

@create3000 create3000 reopened this Jun 2, 2023
@create3000 create3000 added the question Further information is requested label Jun 2, 2023
@vbulatov2011
Copy link

I've just learned about your project and was very excited to find out that X-ITE actually works on mine very convoluted VRML files with extensive scripting and PROTOs which were mostly dead for 20 years. Over weekend I've managed to convert my page of VRML examples http://bulatov.org/vrml into using X_ITE.

That texture+diffuseColor issue was one of very few problems I've encountered. It was minor nuisance easy to fix in the code.
I feel it gives more flexibility to the author.
Very impressive work guys! Thank you!

All the best, Vladimir Buatov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

7 participants