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

HDR Lightmaps & Environment Settings System #4538

Merged
merged 7 commits into from
Sep 2, 2021
Merged

HDR Lightmaps & Environment Settings System #4538

merged 7 commits into from
Sep 2, 2021

Conversation

netpro2k
Copy link
Contributor

@netpro2k netpro2k commented Aug 18, 2021

This PR adds support for RGBE (.hdr) lightmaps and add a system to deal with environment settings. It also introduces the environment-settings component, which for now allows setting tone mapping, exposure, and custom equirectangular environment maps and backgrounds (also optionally HDR).

Currently only a single environment map is supporter per scene, but the plan would be to eventually allow multiple environment maps either by object or area (TBD)

This PR also reworks how references to GLTF types work on MOZ_hubs_components. Now instead of just listing the index, a special marker object is used so that the application loading the GLTF does not need to be aware of the component schema to be able to resolve the reference. This is important as we begin to add more tooling to our pipeline as there will be more things that need to "pass through" component data without breaking it. For now this applies to Spoke and the WIP "Hubs GLB tools".

The marker object looks like:

{ "__mhc_link_type": "texture", "index": 1}

This also begins to transition MOZ_hubs_components implementation to an actual GLTFLoader plugin, though right now does not change any of the existing "inflation" code.

┆Issue is synchronized with this Jira Task

);
}

// TODO decide if thse should get put into the GLTF loader itself
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: typo

Suggested change
// TODO decide if thse should get put into the GLTF loader itself
// TODO decide if these should get put into the GLTF loader itself

@takahirox
Copy link
Contributor

Sorry for delaying the review. Let me do multi posts with the one in Discord so far.

My initial impression for adding the two hooks to the upstream is, I'm on the fence about it. Because we Three.js glTF loader devs don't want to add many hook points. Because we don't want to have an impact to the performance and we want to keep the small complexity We want to add only minimal hook points which primarily help to implement KHR_ prefix extension and some other major extension handlers (of course if the hook points can also be used for generic purpose it's a big bonus point). Hook points are not for implementing all user custom extensions. If the plugin APIs are not fitting to certain custom user extensions, we would like encourage to make a fork instead. From this perspective, I have been thinking whether adding the two hook points is worth or not. And sorry I haven't had a conclusion yet because of the lack of time

@netpro2k netpro2k marked this pull request as ready for review September 1, 2021 02:26
@netpro2k netpro2k merged commit d16a21b into master Sep 2, 2021
@netpro2k netpro2k deleted the hdr-lightmaps branch September 2, 2021 01:06
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.

3 participants