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

Preserve direct light specular reflections for smooth materials #22632

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SeanCurtis-TRI
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI commented Feb 13, 2025

This tweaks the mathematical model so that perfect reflections of our direct lighting don't dwindle away to nothing (counter to how they would behave in the real world).


This change is Reviewable

This tweaks the mathematical model so that perfect reflections of our
direct lighting don't dwindle away to nothing (counter to how they would
behave in the real world).
Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

+@jwnimmer-tri for feature review, although feel free to dispatch this to someone else.

Note: There's no specific unit test. The tutorial uses it, so that is, vaguely, unit-testy. Let me know if you'd like an explicit unit test. I'm pretty sure I can come up with one.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

I don't think we need to add a unit test in this PR. Let's circle back after the tutorial lands and revisit if we want to backfill one.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)


tools/workspace/vtk_internal/patches/preserve_direct_light_specular_reflections.patch line 13 at r1 (raw file):

completely).

In this patch, we're simply mapping the roughness domain [0, 1] to [0.05, 1.0]

BTW We should probably document our reasoning / heuristic / taste for choosing 0.05 vs 0.02 vs 0.08 etc. Is there anything principled? Or are we just looking at 1 (10? 100?) sample render and using our aesthetic judgement? Or are we matching Meshcat's floor?


tools/workspace/vtk_internal/patches/preserve_direct_light_specular_reflections.patch line 50 at r1 (raw file):

+  // remapping input roughness [0, 1] to the range [0.05, 1.0], guaranteeing
+  // that lights create a specular highlight on the smoothest surfaces.
+  return 0.95 * roughness + 0.05;

BTW Do we want 0.95f and 0.05f to keep the compute in float-precision? Maybe it doesn't matter.

@jwnimmer-tri jwnimmer-tri added priority: high release notes: fix This pull request contains fixes (no new features) labels Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants