-
Notifications
You must be signed in to change notification settings - Fork 54
Support HDR (.hdr) images as environment maps #194
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
base: master
Are you sure you want to change the base?
Conversation
- Added loader for hdr images. - Added "Render Settings" control with "exposure" so the final rendering can be better controlled. - Added a hard-coded tone mapper. We can consider exposing it in render settings in the future if we like. - Normalized color environment maps (e.g. png) have been tweaked so the background image is affected by the exposure setting. - Fixed redraw related bugs - Changing the background image/state would lead to a single rendering in which the lighting was incorrect. This was a due to a synchronization error between the rendering system and the image loading. - test/background.html exercises all features. - the glTF cube has been replaced with a sphere to better showcase the environment maps. - The environment maps are now external images (rather than data urls in the html).
+a:@jwnimmer-tri for review, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwnimmer-tri reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @SeanCurtis-TRI)
src/index.js
line 371 at r1 (raw file):
// setting a value will automatically trigger a re-render. When we call // set_property() on the containing SceneNode, we need to trigger a // rerender. SceneNode.set_property() calls this function, to indicate
typo (consistency)
Suggestion:
re-render.
src/index.js
line 1131 at r1 (raw file):
const extension = path.slice((path.lastIndexOf('.') - 1 >>> 0) + 2); let texture = null; switch (extension.toLowerCase()) {
The path here is... a URL? I don't think we can trust that URLs end with human extensions. The drake::geometry::Meshcat
CAS cache doesn't set the extention, does it? Only the mime type.
src/index.js
line 1391 at r1 (raw file):
this.update_background(); this.set_object(["Render Settings"], new RenderConfig());
This causes a console error message.
test/background.html
line 30 at r1 (raw file):
// use_hdr defaults to false; so env map defaults to png. let env_map = env_map_png;
nit Trailing whitespace.
test/background.html
line 52 at r1 (raw file):
}], materials: [ {
nit Trailing whitespace
test/background.html
line 135 at r1 (raw file):
regardless of camera projection type or definition of \ an environment map. The white field still serves as \ environment map, so the box should be brightly lit."
typo
Suggestion:
sphere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @SeanCurtis-TRI)
src/index.js
line 1131 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The path here is... a URL? I don't think we can trust that URLs end with human extensions. The
drake::geometry::Meshcat
CAS cache doesn't set the extention, does it? Only the mime type.
(Also if its a URL it could have ?
or #
trailing the filename extension, I think.)
Related to RobotLocomotion/drake#20366
This change is