fix(batched): Assume BatchedRendererServices texture derivatives are in st space.

AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers

BSD 3-Clause "New" or "Revised" License

2.09k stars 356 forks source link

fix(batched): Assume BatchedRendererServices texture derivatives are in st space. #1828

Closed sfriedmapixar closed 1 month ago

sfriedmapixar commented 3 months ago

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:

[X] I have read the contribution guidelines.
[X] I have updated the documentation, if applicable.
[X] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
[X] 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.

lgritz commented 3 months ago

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?

sfriedmapixar commented 3 months ago

I thought this went through CI cleanly for me, so I'll investigate the diffs.

sfriedmapixar commented 3 months ago

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.

lgritz commented 2 months ago

Sorry for the delay on this.

This look ok to everybody? @AlexMWells @fpsunflower ?

lgritz commented 2 months ago

Is there a simple test we can add that would validate that this is (a) correct, and (b) matches between batch and single point?

fpsunflower commented 2 months ago

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?

lgritz commented 2 months ago

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.