create3000 / x_ite

X_ITE X3D Browser, view and manipulate X3D and VRML scenes in HTML.
https://create3000.github.io/x_ite/
Other
67 stars 13 forks source link

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

Open Sgeo opened 4 years ago

Sgeo commented 4 years ago

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

create3000 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Joining conversations up: https://github.com/x3dom/x3dom/issues/993 The same discussion is being had on x3dom.

create3000 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

https://github.com/create3000/cobweb/issues/34#issuecomment-290920571

michaliskambi commented 4 years ago

From Castle Game Engine / view3dscene ( https://github.com/castle-engine/view3dscene/issues/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 https://github.com/castle-engine/view3dscene/issues/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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

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.

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 commented 4 years ago

@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 commented 4 years ago

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

Sgeo commented 4 years ago

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

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Please note more up-to-date link. We currently await final resolution comments (if any) and publication permission from ISO Secretariat editors.

* https://www.web3d.org/specifications/X3Dv4Draft/ISO-IEC19775-1v4-IS.proof/Pa rt01/components/lighting.html

Thanks for steady impressive efforts. Very respectfully yours.

all the best, Don

--

Don Brutzman Naval Postgraduate School, Code USW/Br @.***

Watkins 270, MOVES Institute, Monterey CA 93943-5000 USA +1.831.656.2149

X3D graphics, virtual worlds, navy robotics https://faculty.nps.edu/brutzman

From: Holger Seelig @.> Sent: Friday, June 2, 2023 7:22 AM To: create3000/x_ite @.> Cc: Brutzman, Donald (Don) (CIV) @.>; Comment @.> Subject: Re: [create3000/x_ite] An appearance with a diffuseColor and an RGB texture should ignore the diffuseColor (#50)

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.

- Reply to this email directly, view it on GitHub https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co m%2Fcreate3000%2Fx_ite%2Fissues%2F50%23issuecomment-1573821100&data=05%7C01% 7Cbrutzman%40nps.edu%7C0df1dc7007eb458d76a208db6374b261%7C6d936231a51740ea91 99f7578963378e%7C0%7C0%7C638213125116104813%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&s data=n1IFTmQCnZrH8dLF9ACfFAikO0xhpQA2VkXClH7jdEg%3D&reserved=0 , or unsubscribe https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co m%2Fnotifications%2Funsubscribe-auth%2FAB23BYA4GDQXSGLCGTYKZ73XJHZHTANCNFSM4 I3Q7USA&data=05%7C01%7Cbrutzman%40nps.edu%7C0df1dc7007eb458d76a208db6374b261 %7C6d936231a51740ea9199f7578963378e%7C0%7C0%7C638213125116104813%7CUnknown%7 CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0 %3D%7C3000%7C%7C%7C&sdata=nEn9v9H%2BM3%2FkYLpTECC4uTk3o9dnDLXWokFPnrKt1RM%3D &reserved=0 . You are receiving this because you commented. https://github.com/notifications/beacon/AB23BYHMLX7VKE4H75IVXDTXJHZHTA5CNFS M4I3Q7USKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOLXHJVLA .gif Message ID: @. @.> >

vbulatov2011 commented 1 year ago

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