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

Playset scene containing lighting #116

Merged
merged 4 commits into from May 24, 2024
Merged

Playset scene containing lighting #116

merged 4 commits into from May 24, 2024

Conversation

rsahlin
Copy link
Contributor

@rsahlin rsahlin commented Apr 17, 2024

A 3D scene created by one of our 3D modellers, it contains a setup with a couple of IKEA items.
The scene has lights and camera to create the desired output.
We would prefer to be able to use environment based lights to emulate diffuse screens in a photoshoot, since this is not present in glTF we have added one directional light as a substitute.

@echadwick-artist
Copy link
Contributor

Thanks for prepping this asset!

  1. The screenshots in the readme seem to be reversed, not matching their respective captions?
  2. The readme seems to be indicating the incorrect extension name for emissive, should be KHR_materials_emissive_strength.
  3. The scene seems to render too brightly in various common glTF renderers (Sample Viewer, Babylonja, Filament, Threejs). Is this intentional? Perhaps something about this in the readme might be helpful.
  4. I wonder if the copyright field in the glTF should match the Text in the metadata.json?

@elalish
Copy link
Contributor

elalish commented Apr 17, 2024

Thanks, this will be a very useful asset for demonstrating lighting units and exposure and such. Would you mind linking your preferred environment HDR here (even if it's not part of the PR)? Assuming it's under the same CC license? I can use that to set this asset up with a good comparison scene in https://github.com/KhronosGroup/glTF-Render-Fidelity once I finally get that up and running.

Screenshot naming is fixed.
Correct name of KHR_materials_emissive_strength.
The 'text' field in metadata is now the same as the copyright field in the glTF.
Added license information in the readme
@lexaknyazev
Copy link
Member

@rsahlin Please rename (via the "Edit" button in the top right corner) the PR title to make it more descriptive, e.g., by using the asset name.

@rsahlin
Copy link
Contributor Author

rsahlin commented Apr 18, 2024

Thanks for reviewing @echadwick-artist - Amazed of the minute details you are able to pick out!!! :-)
Fixed 1, 2 and 4.

As per question 3:
This is a physically correct model with one light using actual values from our products, plus one directional light.
It is a manifestation of the areas that are not covered in the glTF specification:
How 'connect' calculated values to display output - and lack of shutter, aperture and ISO in the camera
:-)

@rsahlin rsahlin changed the title Initial commit Playset model containing lighting Apr 18, 2024
@rsahlin
Copy link
Contributor Author

rsahlin commented Apr 18, 2024

Thanks @elalish

Would you mind linking your preferred environment HDR here
Well - for us the purpose of this model is to show what it must look like when using the features that are part of the standard + Khronos extensions.
For this model it means that we want it to be rendered using black background, ie no ibl, environment map or irradiance map.

@rsahlin
Copy link
Contributor Author

rsahlin commented Apr 18, 2024

@lexaknyazev - Done - yeah, that was a poor name :-)

@elalish
Copy link
Contributor

elalish commented Apr 18, 2024

I was referring to your comment:

We would prefer to be able to use environment based lights to emulate diffuse screens in a photoshoot

An IBL is a necessary part of PBR rendering, as all realistic scenes have some light coming from all directions. It's not included in glTF for the reasons you gave: glTF is a model, not a scene format, and there would be no way to combine different IBLs from models meant to be shown together in one scene. Additionally, the IBL has a significant bandwidth cost that you don't want to pay for over and over when it is reused for multiple assets.

I'll just use one of my own if I have to, but I'd be happy to use yours to look deeper into the lighting unit issues we've been discussing.

@rsahlin
Copy link
Contributor Author

rsahlin commented Apr 19, 2024

This asset is clearly a scene - and a great example of why we need an extension to provide environment information in a specified manner - we simply cannot continue referring to features and data that is not part of the standard.

Each glTF asset is actually a scene - with the possibility to specify multiple scenes if wanted.
Combining environments is only a problem if they lack dimension.
The bandwidth can easily be kept to a minimum using SH.
All and more is solved here:

KhronosGroup/glTF#1956

Once an environment extension is defined, default behavior can be specified in 3D commerce.
It's not the place for 3D formats.
glTF is simply a dataformat, an enabler - going into runtime behavior is a path we should avoid.

I'll just use one of my own if I have to, but I'd be happy to use yours to look deeper into the lighting unit issues we've been discussing.

Please don't use that for this model - the intention is to use standardized features of glTF.
The issues we have been discussing are clearly evident in the model, as is.

@rsahlin rsahlin changed the title Playset model containing lighting Playset scene containing lighting Apr 19, 2024
@echadwick-artist
Copy link
Contributor

It seems that "Directional" is mis-spelled?

Can you indicate which realtime renderer you used? Are there specific settings applied for camera exposure, tone mapping, etc.? Is the renderer available for other people to test against?

It might help to add a screenshot from the glTF Sample Viewer, to help explain the intentions of the asset lighting vs. the current state of glTF rendering in commonly available renderers.

@rsahlin
Copy link
Contributor Author

rsahlin commented May 10, 2024

Thanks @echadwick-artist!
Updated the readme to better explain need for light scale factor.
I am reluctant to add screenshot from sample-viewer since this would just look white....

@echadwick-artist
Copy link
Contributor

Thanks for the edits, I think these help explain the intentions better.

Doesn’t make sense to describe this as a Showcase asset? Showcases are meant to demonstrate positive examples of what the tech can do. But this scene is specifically designed to expose a problem with how we treat lighting units.

@rsahlin
Copy link
Contributor Author

rsahlin commented May 24, 2024

Good thinking @echadwick-artist :-)
I have changed the tag to test, updated folder and name.

@echadwick-artist echadwick-artist merged commit 5c83a75 into main May 24, 2024
4 checks passed
@rsahlin rsahlin deleted the IkeaPlayroom branch May 24, 2024 16:52
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.

None yet

5 participants