Implement basic support for MaterialX closures in testrender #1533
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fpsunflower
force-pushed
the
mx-closure-stubs
branch
3 times, most recently
from
f459abb to
d27d521
Compare
aconty
reviewed
Jul 17, 2022
| float /*rz*/, OSL::Dual2<OSL::Vec3>& wi, | ||
| float& pdf) const | ||
| float& pdf) const override |
There was a problem hiding this comment.
This patch looks good to me, thanks! But I'm going to abuse it to open a discussion on the BSDF API:
- Should we return a struct like { weight, pdf } to avoid output args?
- Can we drop the full sg as argument and instead pass a Vec3 wo (meaning I) so you can evaluate them in an SG-less context?
- Does anybody need accurate derivs from wi? If not, can we stop pretending we can compute them?
- Should we add roughness to the output? I believe most renders track it. You can guess it from pdf following a convention (like reversing an isotropic GGX), but wanted to get opinions. A normalized [0-1] roughness can be used as a measure of specularity for AOVs. If we sign off on BSDFs being single lobe the roughness can be just a BSDF property and not an output from sample()/eval().
I volunteer to making any of these changes after you merge this, of course.
There was a problem hiding this comment.
- Yes - good idea. I can do that refactor in another PR. Would you have a { weight, pdf, dir } struct for sample() then?
- Yes - good idea.
- Yes - definitely. Only the reflection() and refraction() cases do anything sensible. Do you want to submit a patch that switches to ray cones? That would be a nice simplification for the bsdf API.
- I wouldn't bother with this for now. I prefer keeping testrender minimal for now.
There was a problem hiding this comment.
Yes { color, wi, pdf } in any order you like works for me. I will do the cone tracing.
aconty
reviewed
Jul 17, 2022
| } | ||
| case MX_SHEEN_ID: | ||
| case MX_LAYER_ID: { | ||
| OSL_ASSERT(false && "MaterialX closure not yet implemented"); |
There was a problem hiding this comment.
I have a suggestion on how to implement this. Add a
virtual Color3 filter_base(Vec3 wo);
You call it here, scale the weight of base and just add_bsdf() both. Recurse where needed. Works for us by making assumptions on wi. Eliminates the need of recursion at eval/sample time. De-virtualize and you are ready for GPU.
There was a problem hiding this comment.
Thats what I was thinking too. Maybe it can even just use the existing "get_albedo()" method?
There was a problem hiding this comment.
Good idea, although the coating may want to add absorption according to some thickness. So maybe a different method that by default does 1 - get_albedo().
fpsunflower
force-pushed
the
mx-closure-stubs
branch
from
d27d521 to
ee1d955
Compare
* Extend the bsdf interface in testrender to use Color3 instead of float in preparation for MaterialX closures which are almost all tinted * Add stub parameters and IDs for all MaterialX closures * Register all MaterialX closure types * Implement most MaterialX closures, reusing the existing implementations for now This commit is mainly intended to do the busy work of adding the new closures, no new BSDFs are actually implemented yet. This means that some of the closures are handled in a very rudimentary way, ignoring some key parameters. The intent is to improve support for these over time. Signed-off-by: Chris Kulla <[email protected]>
fpsunflower
force-pushed
the
mx-closure-stubs
branch
from
ee1d955 to
35a2364
Compare
|
I haven't found any workaround yet ... I'll keep updating |
|
Any further comments on this PR? |
aconty
approved these changes
Jul 19, 2022
|
LGTM! |
lgritz
pushed a commit
to lgritz/OpenShadingLanguage
that referenced
this pull request
Jul 23, 2022
…SoftwareFoundation#1533) * Extend the bsdf interface in testrender to use Color3 instead of float in preparation for MaterialX closures which are almost all tinted * Add stub parameters and IDs for all MaterialX closures * Register all MaterialX closure types * Implement most MaterialX closures, reusing the existing implementations for now This commit is mainly intended to do the busy work of adding the new closures, no new BSDFs are actually implemented yet. This means that some of the closures are handled in a very rudimentary way, ignoring some key parameters. The intent is to improve support for these over time. Signed-off-by: Chris Kulla <[email protected]>
chellmuth
pushed a commit
to chellmuth/OpenShadingLanguage
that referenced
this pull request
Sep 6, 2024
Relevant changes: * GPU string fix -- needed missing cast (AcademySoftwareFoundation#1553) * Fix typo error that prevented correct typecheck of ternary (AcademySoftwareFoundation#1552) * testrender now supports the standard MaterialX closure: * testrender improvements: modernize sampler (AcademySoftwareFoundation#1534), switch to cone tracing (AcademySoftwareFoundation#1543), support MaterialX closures (AcademySoftwareFoundation#1533, AcademySoftwareFoundation#1536, AcademySoftwareFoundation#1537, AcademySoftwareFoundation#1538, AcademySoftwareFoundation#1541, AcademySoftwareFoundation#1542, AcademySoftwareFoundation#1547) * Bump LLVM to 14.0 * Help reduce PTX nondeterminism (AcademySoftwareFoundation#1570) See merge request spi/dev/3rd-party/osl-feedstock!30
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend the bsdf interface in testrender to use Color3 instead of float in preparation for MaterialX closures which are almost all tinted
Add stub parameters and IDs for all MaterialX closures
Register all MaterialX closure types
Implement most MaterialX closures, reusing the existing implementations for now
This commit is mainly intended to do the busy work of adding the new closures, no new BSDFs are actually implemented yet. This means that some of the closures are handled in a very rudimentary way, ignoring some key parameters. The intent is to improve support for these over time.