-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add emission_weight #231
Add emission_weight #231
Conversation
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.
I approve.
|
Ah right, I see the parameter list still has a luminance default value of 0 (assuming you meant The default value 1000 sounds reasonable to me since it's the order of magnitude of the maximum luminance of typical displays screens (smartphones and TVs). |
Co-authored-by: Julien Guertault <9511025+virtualzavie@users.noreply.github.com> Signed-off-by: Jamie Portsmouth <jamports@mac.com>
This looks fine to me, though it would be ideal to update the reference implementation in sync with this change, so that we don't accidentally create divergence between them. |
Done in eed5022 |
@jstone-lucasfilm can be merged? |
@portsmouth This looks very close to the mark, but we should update Anton's https://github.com/AcademySoftwareFoundation/OpenPBR/blob/main/examples/open_pbr_lightbulb.mtlx |
Done in 203103e. Hopefully there aren't a lot of existing assets out there that will be bitten by this (I assume not, yet..). |
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.
This looks good to me, thanks @portsmouth!
297d447
into
AcademySoftwareFoundation:dev_1.2
Implements the change described in #229.