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

INTEL_lights_sunsky: a procedural sun-sky model as environmental light source #2179

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

Conversation

johguenther
Copy link

Procedural sun-sky models are a wide-spread and flexible tools to provide natural illumination to scenes and objects or commercial products, complementing point lights (KHR_lights_punctual) and image-based environment maps (#1956).

@johguenther
Copy link
Author

Any feedback/discussion/review?

@BruceCherniak
Copy link

There has been no feedback on this since Jun. I support this extension, and it is critical to some of our customers. Am I able to approve the "review" as a vendor extension without further discussion?

@lexaknyazev
Copy link
Member

To move this further, the PR needs JSON schemas for the extension objects.

@donmccurdy
Copy link
Contributor

I feel it would be useful to publish some visibility into what to expect when opening a PR for a {vendor, multi-vendor, or KHR} extension. There should be nothing blocking the authoring vendor from using their own vendor extension, with or without review. I assume the review process exists to ensure quality and consistency of documents published under the Khronos repository, as well as to offer technical feedback (if desired by the vendor).

@johguenther
Copy link
Author

To move this further, the PR needs JSON schemas for the extension objects.

JSON schemas are now included.

@BruceCherniak
Copy link

To move this further, the PR needs JSON schemas for the extension objects.

JSON schemas are now included.

Intel is still interested in merging this vendor extension.

@javagl
Copy link
Contributor

javagl commented May 7, 2024

I cannot say anything from the perspective of a possible implementation. From quickly skimming over the text and the schema, it looks "clean", but that's not a thorough review.

A few high-level comments:

  • The role of the node that the light is attached to might have to be clarified. Specifically: How does the node transform affect the light? I remember that this always raised questions elsewhere - for punctual lights and other things (even cameras).
  • I wonder whether something should be said about combining multiple lights. Maybe that's obvious or a non-issue, as in "Just perform the computations - the specification is not responsible for defining what 'makes sense', but only to offer all the options"...
  • The text mentions some horizonExtension parameter. I wonder whether this should be part of the schema...?

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

Successfully merging this pull request may close these issues.

6 participants