Skip to content
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

support GL_ARB_texture_multisample extension. #3430

Merged
merged 3 commits into from
Dec 28, 2023

Conversation

jimihem
Copy link
Contributor

@jimihem jimihem commented Nov 29, 2023

This extension allows the use of "texelFetch" and "textureSize" with 2DMS sampler.
#version 140
out float result;

uniform sampler2DMS data;
uniform sampler2DMSArray data1;
void main()
{
result = texelFetch(data, ivec2(0), 3).r;
ivec2 temp = textureSize(data);
result = texelFetch(data1, ivec3(0), 3).r;
ivec3 temp1 = textureSize(data1);
}

Copy link
Collaborator

@ncesario-lunarg ncesario-lunarg left a comment

Choose a reason for hiding this comment

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

I think this looks good apart from the test file.

Test/GL_ARB_texture_multisample.vert Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not the desired output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your reminder, I have update the test file and expected result file.


if (builtIn && (fnCandidate->getBuiltInOp() == EOpTextureFetch || fnCandidate->getBuiltInOp() == EOpTextureQuerySize)) {
TString typeName = (*fnCandidate)[0].type->getSampler().getString();
if ((typeName == "sampler2DMS" || typeName == "sampler2DMSArray") && version == 140)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this makes sense after looking at the spec, but just to clarify, this extension is only required for version 140? i.e., it is not possible to use this extension prior to 140?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggesstion, I have made modifications to this PR.

This extension allows the use of "texelFetch" and "textureSize" with 2DMS sampler.
@jimihem jimihem force-pushed the GL_ARB_texture_multisample branch from 0b6cb32 to 671c1d3 Compare November 30, 2023 03:11
Comment on lines 10 to 13
result = texelFetch(data, ivec2(0), 3).r;
ivec2 temp = textureSize(data);
result = texelFetch(data1, ivec3(0), 3).r;
ivec3 temp1 = textureSize(data1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be nice if these were indented. Not sure if we want that enforced @arcady-lunarg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have organized the format @ncesario-lunarg

Copy link
Collaborator

@ncesario-lunarg ncesario-lunarg left a comment

Choose a reason for hiding this comment

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

One question, but LGTM.

Looks like there are really 2 distinct changes here; I'm not sure how they are related?

glslang/MachineIndependent/ParseHelper.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@arcady-lunarg arcady-lunarg left a comment

Choose a reason for hiding this comment

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

Could you add tests for usampler2DMS and isampler2DMS just for completeness? Also, I think it would be better not to construct the sampler type string and just check the TSampler's properties directly via the appropriate accessors.

@jimihem jimihem force-pushed the GL_ARB_texture_multisample branch from ea5bcdf to f507bbd Compare December 27, 2023 06:03
Copy link
Contributor

@arcady-lunarg arcady-lunarg left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@arcady-lunarg arcady-lunarg merged commit 88c5373 into KhronosGroup:main Dec 28, 2023
22 checks passed
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