Skip to content

fix: pass the real number of channels to rs_texture and rs_texture3d on gpu #2004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 2, 2025

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Jul 1, 2025

These were discovered when moving SPI's renderer to the the current release and switching to use the rs_ functions.

These were discovered when moving SPI's renderer to the the current
release and switching to use the rs_ functions.

Signed-off-by: Larry Gritz <[email protected]>
@lgritz
Copy link
Collaborator Author

lgritz commented Jul 1, 2025

The Windows tests are failing because of unrelated issue #2003.

I think I've made a mistake in the OptiX parts, actually. Don't review yet, I think this still needs a little work.

@lgritz
Copy link
Collaborator Author

lgritz commented Jul 1, 2025

Eew, there is some confusion about how the rs_texture family of functions should work. The parameters are passed as (int nchans, float* result) (I'm simplifying), and in some parts of the code, we act as if result was an array of nchans, but in other places we seem to assume that it's always a pointer to float[4], and it's up to the caller to later copy out individual floats if needed.

It might be worth noting that for the old RendererServices, the texture methods also pass int nchans, float* result and expect that it's result[nchans]. But WITHIN the implementation, there is a lower level that assumes float[4] in order to use some SIMD acceleration.

So do we imagine rs_texture to be a direct substitute (in a sense) for RenderServices::texture(), or is it more analogous to the hidden thing in side RS::texture that knows it has a float[4] to deal with? Which would you like it to be?

Now that I've written the above, I think that it makes most sense for rs_texture to mimic RendererServices::texture, in that the fact that we pass nchans at all makes me think that it's supposed to only touch result[0..nchans-1] and not just jam a float4 in there.

But if you disagree strongly, speak up before I finish fixing it all.

@lgritz
Copy link
Collaborator Author

lgritz commented Jul 1, 2025

Never mind that last comment. I think I have it all fixed already, was just one spot making a bad assumption. Obviously, rs_texture ought to have the same meaning of its parameters as RendererServices:texture did.

Signed-off-by: Larry Gritz <[email protected]>
@lgritz lgritz requested review from chellmuth and aconty July 1, 2025 22:51
@lgritz
Copy link
Collaborator Author

lgritz commented Jul 1, 2025

@chellmuth @aconty This is a necessary step to get Spear fully caught up.

Copy link
Contributor

@aconty aconty left a comment

Choose a reason for hiding this comment

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

Good catch!

@lgritz lgritz merged commit 67693b9 into AcademySoftwareFoundation:main Jul 2, 2025
24 of 26 checks passed
@lgritz lgritz deleted the lg-rstexture branch July 2, 2025 16:47
lgritz added a commit to lgritz/OpenShadingLanguage that referenced this pull request Jul 2, 2025
…on gpu (AcademySoftwareFoundation#2004)

These were discovered when moving SPI's renderer to the the current
release and switching to use the rs_ functions.

Signed-off-by: Larry Gritz <[email protected]>
Copy link
Contributor

@sfriedmapixar sfriedmapixar 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.

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