Skip to content

Conversation

fpsunflower
Copy link
Contributor

Description

We intended this closure to take the effective color and radius so that the SSS color remapping can be controlled by the renderer.

Tests

The implementation in testrender uses a simple diffuse response (which was effectively assuming it got a multiple-scattered albedo. So all that was needed was renaming the parameters and updating docs.

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.

…s instead of single scattering parameters

Signed-off-by: Chris Kulla <ckulla@gmail.com>
@fpsunflower
Copy link
Contributor Author

@jstone-lucasfilm @niklasharrysson this is for addressing the issue you brought up on slack. Let me know if the documentation looks ok.

@lgritz The documentation exists in 3 places. I'm guessing the latex docs are deprecated, but is there anyway for the new md docs to use the header file somehow?

@lgritz
Copy link
Collaborator

lgritz commented Jun 10, 2024

The src/doc/*.md (particularly stdlib.md) is the primary documentation. The LaTeX is obsolete, and exists now only for us to double-check that the new docs have faithfully reproduced what we have.

We don't yet have a way to generate docs from the headers, but it sure would be nice. (Open question: whether Doxygen can be coaxed into parsing osl headers so that we can use Sphinx + Breath + Doxygen like we do for the OIIO docs.)

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM. Is this ready to go?

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm 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 to me!

Copy link
Contributor

@niklasharrysson niklasharrysson 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 to me too. Thanks @fpsunflower!

@fpsunflower fpsunflower merged commit 02e2bc3 into AcademySoftwareFoundation:main Jun 11, 2024
@fpsunflower fpsunflower deleted the fix-sss-closure-params branch June 11, 2024 08:18
chellmuth pushed a commit to chellmuth/OpenShadingLanguage that referenced this pull request Sep 6, 2024
* fix: reparameter string needed to be ustringhash (AcademySoftwareFoundation#1841)
* fix: make backfacing shadeop indicate backfacing shader-global is needed (AcademySoftwareFoundation#1827)
* Fix the subsurface_bssrdf parameters (AcademySoftwareFoundation#1823)
* Implement OpenPBR's sheen BRDF in testrender (AcademySoftwareFoundation#1819)
* cleanup: OIIO deprecations (AcademySoftwareFoundation#1834)

See merge request spi/dev/3rd-party/osl-feedstock!61
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.

4 participants