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

Add a test of light intensity vs. color #163

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

emackey
Copy link
Member

@emackey emackey commented Jan 8, 2025

@lexaknyazev Is this similar to what you had in mind?

@emackey
Copy link
Member Author

emackey commented Jan 8, 2025

Follows KhronosGroup/glTF#2215

@lexaknyazev
Copy link
Member

The spec language means that the specified color works as a "filter" in front of a light source of the specified intensity. So we need two more groups of planes to further assert the expected behavior (they all could be in the same asset for convenience):

Two coplanar squares next to each other. One white point light is located above the center of the first square; three point lights: red, green, and blue, are co-located above the center of the second plane. All intensities must be the same.

Two coplanar squares next to each other. One point light with color (1, 1, 1) is located above the center of the first square; another point light with color (0.5, 0.5, 0.5) is located above the center of the second plane. Both intensities are the same.

@javagl
Copy link
Contributor

javagl commented Jan 8, 2025

This sounds somewhat similar to what I did for #78 (comment) (and yes, shame on my, I never opened that PR...)

If desired, I can allocate a bit of time to (really) create such a model. With the nitpick: Is the part of having separate squares relevant for the actual comparison? (It might be, roughly as in "The squares must look equal", as opposed to "The reflections of the light on the surface must look equal").

(And an aside: Auto-generating such a scene would also avoid these
"intensity":1.0000660226034708,
that make my eye twitch sooo bady 😆 )

@lexaknyazev
Copy link
Member

Is the part of having separate squares relevant for the actual comparison?

I think that having distinct gaps between squares would make the differences (or the lack of them) more compelling,

@javagl
Copy link
Contributor

javagl commented Jan 9, 2025

@emackey I hope I'm not stepping on your toes here.

This would be unit squares with base color (1,1,1) (ok?)
Point lights are placed at y=0.5 at the respective center above them.
All lights have an intensity of 1.0.
The colors are given (verbally) in the labels. I.e. in the upper right, there are three lights. And "Gray" means (0.5, 0.5, 0.5) here.

LightsPunctual Intensity 2025-01-09

The tweaking for the settings in the viewer may have to be explained in the README. Otherwise, users might see the model with default settings:

LightsPunctual Intensity defaults 2025-01-09

The model is here, just for reference for now:

LightsPunctual Intensity 2025-01.09.glb.zip

@emackey
Copy link
Member Author

emackey commented Jan 10, 2025

@javagl I don't mind which of these models gets merged, whatever works best.

I used (0.8, 0.8, 0.8) as the base color of my test material. I've heard a general rule-of-thumb for PBR that says one should not typically go higher than this in any channel of the base color, to allow for specular highlights and hotspots to have some headroom without leaning completely on tone mapping. But clearly this rule-of-thumb is for photorealistic objects, not test scenarios like we have here. Even so, with light sources present, I thought I would use 0.8 gray for the test surface.

@emackey
Copy link
Member Author

emackey commented Jan 10, 2025

screenshot-large

@javagl
Copy link
Contributor

javagl commented Jan 10, 2025

That certainly looks nicer. If this covers the relevant cases (which, to my understanding, it does), then that's better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants