-
Notifications
You must be signed in to change notification settings - Fork 379
Add GLSL support for SampleCmpBias and SampleCmpGrad #9085
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
base: master
Are you sure you want to change the base?
Conversation
…sl.meta.slang Co-authored-by: davli-nv <219758217+davli-nv@users.noreply.github.com>
Co-authored-by: davli-nv <219758217+davli-nv@users.noreply.github.com>
|
|
||
| [ForceInline] | ||
| [require(glsl_hlsl_spirv, texture_shadowlod)] | ||
| [require(glsl_hlsl_spirv, texture_shadowgrad)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the require for existing untouched texture* functions in glsl.meta.slang should not change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted the require change for functions without bias parameters that don't transitively call SampleCmpBias. Note that textureProjOffset(sampler2DShadow, vec4, offset) still needs texture_shadowgrad since it calls textureOffset with default bias=0.0. (26fe990)
source/slang/hlsl.meta.slang
Outdated
| __target_switch | ||
| { | ||
| case glsl: | ||
| __intrinsic_asm "textureGrad"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GLSL textureGrad etc have different parameter order and semantics than HLSL intrinsics, might need to use wrappers here similar to __glsl_texture_1d_shadow etc, and add wrappers as needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added GLSL wrapper functions __glsl_texture_grad_shadow and __glsl_texture_grad_offset_shadow similar to existing __glsl_texture_1d_shadow pattern. Updated SampleCmpGrad methods to use these wrappers that handle the GLSL parameter format. (26fe990)
source/slang/glsl.meta.slang
Outdated
| case spirv: | ||
| return spirv_asm | ||
| { | ||
| result:$$float = OpImageSampleDrefImplicitLod $sampler $location $compareValue Bias|ConstOffset $bias $offset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the glsl and spirv cases can call appropriate sampler.SampleCmp* overloads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified texture functions to call sampler.SampleCmpBias directly for all targets instead of using __target_switch with separate glsl/spirv cases. This applies to texture(), textureOffset(), and their variants with bias parameters. (26fe990)
|
|
||
| [ForceInline] | ||
| [require(glsl_hlsl_spirv, texture_shadowlod)] | ||
| [require(glsl_hlsl_spirv, texture_shadowgrad)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this require change is probably needed to call sampler.SampleCmpBias
|
@copilot address my review comments |
…s, fix capability requirements Co-authored-by: davli-nv <219758217+davli-nv@users.noreply.github.com>
| @@ -0,0 +1,81 @@ | |||
| // Test GLSL output for SampleCmpBias and SampleCmpGrad functions | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this new test file is not needed, there is existing test of glsl texture* function overloads.
need to add glsl checks into existing tests for SampleCmpGrad and SampleCmpBias overloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the new test file and added CHECK_GLSL checks to the existing tests/hlsl-intrinsic/texture/sample-cmp.slang test for SampleCmpBias and SampleCmpGrad calls. (0b68ed9)
source/slang/hlsl.meta.slang
Outdated
| __target_switch | ||
| { | ||
| case glsl: | ||
| __intrinsic_asm "texture"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the arguments of this call to glsl intrinsic texture() correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - the GLSL case now properly calls wrapper function __glsl_texture_bias_shadow(this, __makeVector(location, compareValue), bias) which formats parameters correctly for GLSL's texture() intrinsic. Added wrappers __glsl_texture_bias_shadow and __glsl_texture_bias_offset_shadow similar to the existing __glsl_texture_grad_shadow pattern. (0b68ed9)
|
@copilot address my latest review |
…move redundant test file Co-authored-by: davli-nv <219758217+davli-nv@users.noreply.github.com>
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.