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

fixed-client-target: changed client target setting, following glslangValidator approach #47

Merged
merged 2 commits into from
Jul 31, 2023

Conversation

mixnam
Copy link
Contributor

@mixnam mixnam commented Jul 17, 2023

I was trying to solve these issues

As I found, currently we are setting envClient and envTarget every time, no matter what kind of client we user specified via cli args.
However it might be a wrong approach. In glsllang repo I noticed that for OpenGL we should not set these properties at all .

I was trying to implement same logic here, and bound EShMessages messages with target client version specified via cli args.

NOTE:
I don't have any experience in c++ and contribution to open source, please don't blame me too much. And I would more than appreciate any help from anybody.

@mixnam mixnam marked this pull request as ready for review July 17, 2023 16:02
@mixnam mixnam force-pushed the fixed-client-target branch from 99d0ca4 to ed9b5f3 Compare July 17, 2023 19:18
@svenstaro svenstaro requested a review from nolanderc July 19, 2023 00:38
@svenstaro
Copy link
Owner

@nolanderc wanna take a look?

@nolanderc
Copy link
Collaborator

Looks good! Thank you!

@nolanderc nolanderc merged commit da51473 into svenstaro:master Jul 31, 2023
@jcorbin
Copy link

jcorbin commented Oct 8, 2023

Hey there, any chance we could get a new release tag cut with this change included? Just confirmed by manually building master branch that this change makes glslls viable for a webgl2 project; without this, its diagnostics are misaligned with what webgl2 wants.

@svenstaro
Copy link
Owner

Ok here you go.

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.

4 participants