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

Allow users to bring their own lights #34

Closed
jsantell opened this issue Oct 23, 2018 · 6 comments
Closed

Allow users to bring their own lights #34

jsantell opened this issue Oct 23, 2018 · 6 comments

Comments

@jsantell
Copy link
Contributor

While some content publishers want full control over the scene, there are specifically perf costs for things like shadows and lights. Do we want something like allow-model-lights to import dynamic lights that are found in a glTF model? Would this enable shadows as well? Should this be disabled, even if set, on mobile?

@jsantell
Copy link
Contributor Author

jsantell commented Oct 23, 2018

I don't think dynamic lights is a good idea for several reasons (more in #13) and it seems that the 2.0 extension for lights is in flux (#35). Pinging Don for more info.

Edit: 2.0 light extension is supported by GLTFLoader

@cdata
Copy link
Contributor

cdata commented Oct 24, 2018

My take on this is that we should have two settings. The primary (and default) setting causes lights in the GLTF to be removed, and replaced with whatever built-in lighting / envmap scheme we come up with. The alternative (user configurable) setting retains the lights in the GLTF.

The rationale here is that the default setting puts users on the good path that is likely to lead to a good performance and consistency outcome, while the user configurable alternative allows a user to say, "no, really, I know what I am doing, please give me the lights that are in the GLTF." In other words, we give users a good default, plus an unconstrained escape hatch.

@donmccurdy
Copy link
Contributor

+1 for ignoring lights by default (even though the extension is pretty close to finalized).

Also consider logging a warning when punctual lights are ignored — perhaps even going so far as to say that including lights in an asset is "generally not recommended" or some such thing. I think (but would be willing to discuss) that env maps should similarly be ignored by default.

In general, I am hoping that DCC tool exports and model libraries like Poly will not include lights or environment maps by default, and these defaults can give a slight nudge in that direction.

@jsantell
Copy link
Contributor Author

@donmccurdy is there envmap support in gltf now?

@donmccurdy
Copy link
Contributor

No, only as a vendor extension: https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Vendor/EXT_lights_image_based. I think we would want to discuss further before implementing it at this stage.

@cdata cdata changed the title Allow lights? Allow users to bring their own lights Nov 16, 2018
@elalish
Copy link
Collaborator

elalish commented Mar 30, 2021

Done ages ago

@elalish elalish closed this as completed Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants