Skip to content

Conversation

@jstone-lucasfilm
Copy link
Member

Description

This changelist proposes improvements to the energy conservation between layered BSDFs in OSL.

The rational quadratic fit for GGX directional albedo from the MaterialX project has been integrated into dielectric_bsdf and generalized_schlick_bsdf, improving visual parity with MaterialX GLSL and MDL.

Tests

Here are the latest render comparisons between GLSL and OSL, using the codebase in this PR:

MaterialXRenderTests_OSL_Energy_Conservation_GitHub.pdf

Checklist:

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project. If I haven't
    already run clang-format v17 before submitting, I definitely will look at
    the CI test that runs clang-format and fix anything that it highlights as
    being nonconforming.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 4, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: lgritz / name: Larry Gritz (e2592d1, 0cc1799)
  • ✅ login: jstone-lucasfilm / name: Jonathan Stone (1b42acd)

Copy link
Contributor

@fpsunflower fpsunflower left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for doing this.

Should be fine to merge once you get the CLA sorted out :)

@lgritz
Copy link
Collaborator

lgritz commented Feb 6, 2025

@jstone-lucasfilm I took a pass at making updated ref images (and a clang-format pass) that gets this PR to the point of passing all CI tests. Is it ok with you if I push it to your tree to update the patch?

Also... you seem to have made this PR directly from your fork's "main" instead of a topic branch.

@jstone-lucasfilm
Copy link
Member Author

@lgritz Yes, feel free to make those changes directly in my fork of OSL.

For historical reasons, I have the habit of working in main until I need a second development branch, though I know that this isn't the standard GitHub flow.

@lgritz
Copy link
Collaborator

lgritz commented Feb 7, 2025

Update pushed

@jstone-lucasfilm
Copy link
Member Author

Thanks to Francois and our legal team, Lucasfilm is now approved to contribute to OSL!

Let me know if any additional adjustments are needed for this change.

@lgritz
Copy link
Collaborator

lgritz commented Feb 7, 2025

I think you need to affix your DCO properly to your commit.

@lgritz
Copy link
Collaborator

lgritz commented Feb 7, 2025

Don't worry about those two MacOS CI failures -- they are the very thing addressed by #1936 and are unrelated to this PR.

This changelist proposes improvements to the energy conservation between layered BSDFs in OSL.

The rational quadratic fit for GGX directional albedo from the MaterialX project has been integrated into `dielectric_bsdf` and `generalized_schlick_bsdf`, improving visual parity with MaterialX GLSL and MDL.

Signed-off-by: Jonathan Stone <[email protected]>
@jstone-lucasfilm
Copy link
Member Author

Thanks @lgritz -- I've added the DCO language and force-pushed my single commit. Feel free to adjust if any subsequent commits ended up being lost.

I'm not used to including DCO language in commits, as we intentionally omit this on the MaterialX project, but I'll aim to remember to include them for future OSL changes.

@lgritz
Copy link
Collaborator

lgritz commented Feb 7, 2025

For DCO, you just need to locally commit with git commit -s. There's a way to rig it so it always does this even if you omit the -s (I think we explain this in our CONTRIBUTING).

I'm curious -- why does MaterialX not use DCO?

@jstone-lucasfilm
Copy link
Member Author

@lgritz In discussions so far, the conclusion has been that it's more important to protect contributors from automated collection of their real-world addresses on GitHub, so we've maintained the original no-DCO policy that we started with as an open-source project under Lucasfilm.

We're definitely open to new ideas here, though, and I'm sure there's a balance of advantages and disadvantages on both sides.

@lgritz
Copy link
Collaborator

lgritz commented Feb 7, 2025

Oops, I believe you repushed ONLY your original commit, losing all the fixes I did on top of it to update the ref images.

@lgritz
Copy link
Collaborator

lgritz commented Feb 7, 2025

I'll re-apply my other things on top and see if that clears it all up.

Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
@jstone-lucasfilm
Copy link
Member Author

@lgritz That's my fault, and I wasn't sure how to force-push my own commit without affecting yours. Perhaps a squash merge of both of our changes, followed by a force push?

@lgritz
Copy link
Collaborator

lgritz commented Feb 7, 2025

I think probably the easiest thing would have been

git rebase -i HEAD~3

then change 'r' for your commit and 's' for mine, and when you get to re-enter your commit message, just add the "Signed-off-by:" line by hand.

Hang on, I think I can fix it all from my end.

@lgritz
Copy link
Collaborator

lgritz commented Feb 7, 2025

Yeah, it would also have been fine to squash them all together and then force-push. Anyway, luckly I still had my changes locally, so was trivial to just cherry-pick them on top of yours again.

@lgritz lgritz merged commit ac63a5a into AcademySoftwareFoundation:main Feb 7, 2025
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants