fix(batched): Assume BatchedRendererServices texture derivatives are in st space. #1828
Conversation
…in st space. The convention in the single-point RendererServices is that the texture call returns derivatives in st space, and they are transformed to xy space before returning from the wrapper to RenderServices. This change makes BatchedRendererServices follow the same convention. Signed-off-by: Stephen Friedman <[email protected]>
| MaskedData resultRef = outputs.result(); | ||
| bool has_derivs = resultRef.has_derivs(); | ||
| // Matching scalar code path behavior to check only result derivs. | ||
| ASSERT(has_derivs); |
There was a problem hiding this comment.
I think this should be OSL_ASSERT if you want to check all the time, OSL_DASSERT if you only want to check for debug builds. I believe the ASSERT is just an old alias.
There was a problem hiding this comment.
I went with OSL_DASSERT for the asserts I left -- in production if the types don't match because we start returning new types, those types will get mis-scaled derivatives, but shouldn't crash, and so I think the debug assert for the developer to notice is appropriate.
|
Yay for fixing to make the batch match the single point shading for texture derivs. Seems to be failing a bunch of the CI. Do reference outputs need to be updated as part of this? |
|
I thought this went through CI cleanly for me, so I'll investigate the diffs. |
…ive spaces match the single point assumptions. This finishes the logic surrounding checking alpha for derivatives and applies the fix to the default texture3d implementation. Signed-off-by: Stephen Friedman <[email protected]>
Signed-off-by: Stephen Friedman <[email protected]>
|
sigh This is why we have CI, and why you should always let your local CI finish before making a pull request. I just had gotten interrupted and hadn't finished getting the changes for alpha and texture3d(). Now it's a complete change and passing CI. |
|
Sorry for the delay on this. This look ok to everybody? @AlexMWells @fpsunflower ? |
|
Is there a simple test we can add that would validate that this is (a) correct, and (b) matches between batch and single point? |
|
This looks ok to me. I assume the testsuite already has something that covers this since there were some CI failures before the patch was complete? |
|
I think the reason this was wrong all along (for batch, but correct for single point) is that we don't have a test that ensures that the derivatives of the texture results are correct. I've been fighting a lot of CI failures lately -- github changed some things, OIIO changed some things. Only recently that I've got everything passing again, so it's not at all surprising that CI was broken (for reasons unrelated to this PR) when it was first submitted but is ok now. |
…in st space. (AcademySoftwareFoundation#1828) The convention in the single-point RendererServices is that the texture call returns derivatives in st space, and they are transformed to xy space before returning from the wrapper to RenderServices. This change makes BatchedRendererServices follow the same convention. --------- Signed-off-by: Stephen Friedman <[email protected]>
The convention in the single-point RendererServices is that the texture call returns derivatives in st space, and they are transformed to xy space before returning from the wrapper to RenderServices. This change makes BatchedRendererServices follow the same convention.
This is still tested by testrender/testshade, but the code motion means the assumptions visible at the renderer/OSL boundary are now consistent between batched/non-batched renderservices.
Checklist:
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.