shaders preprocessor messup with GLSL texture functions (texture -> texMap String replace) #3961

Closed
stmaccarelli opened this Issue Oct 5, 2015 · 13 comments

Comments

Projects
None yet
3 participants
@stmaccarelli

Eg. textureOffset() becomes texMapOffset()
I'm using a workaround (by letting preprocessor replace texMapCube to texture, so texMapCubeOffset() becomes textureOffset()

I think you could simply add the texture functions to Search and Replace Strings in the shaders pre-processor setup

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Oct 6, 2015

Contributor

You should have #version directive in your shaders. The preprocessor is there to preserve compatibility with older GLSL 110 shaders which were used in Processing 2. If you have #version directive, we leave your code alone, otherwise we assume it is old 110 code and we preprocess it so it can run at all.

Contributor

JakubValtar commented Oct 6, 2015

You should have #version directive in your shaders. The preprocessor is there to preserve compatibility with older GLSL 110 shaders which were used in Processing 2. If you have #version directive, we leave your code alone, otherwise we assume it is old 110 code and we preprocess it so it can run at all.

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Oct 6, 2015

Member

But the function textureOffset is 1.30+, so it will still create conflicts if used. Sorry, you are right. Adding the #version directive should solve this problem.

Member

codeanticode commented Oct 6, 2015

But the function textureOffset is 1.30+, so it will still create conflicts if used. Sorry, you are right. Adding the #version directive should solve this problem.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Oct 6, 2015

Contributor

@codeanticode I will add it to the replacement map. However, if there is no #version, we assume the code is 110 (or 120 at most) and there should be no textureOffset in the first place. We should probably clarify if users should use #version or not and what GLSL version we support. Then modify the preprocessor according to that.

Contributor

JakubValtar commented Oct 6, 2015

@codeanticode I will add it to the replacement map. However, if there is no #version, we assume the code is 110 (or 120 at most) and there should be no textureOffset in the first place. We should probably clarify if users should use #version or not and what GLSL version we support. Then modify the preprocessor according to that.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Oct 6, 2015

Contributor

@codeanticode There seems to be a lot of different textureXXX functions in 440 so I will add a regular expression to the preprocessor to deal with all of them at once. Then we should be able to preprocess from more versions.

Contributor

JakubValtar commented Oct 6, 2015

@codeanticode There seems to be a lot of different textureXXX functions in 440 so I will add a regular expression to the preprocessor to deal with all of them at once. Then we should be able to preprocess from more versions.

@JakubValtar JakubValtar reopened this Oct 6, 2015

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Oct 6, 2015

Member

Wait... I don't think we need to add any of this to the replacement map, as you pointed out none of these functions are part GLSL <130

Member

codeanticode commented Oct 6, 2015

Wait... I don't think we need to add any of this to the replacement map, as you pointed out none of these functions are part GLSL <130

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Oct 6, 2015

Contributor

However different OpenGL version we can be running on support different minimum version of GLSL, so we should also be able to convert e.g. from 130 to 440, so users don't have to deal with this. However if you say we support only 120 shaders in Processing, then indeed it is not needed.

Contributor

JakubValtar commented Oct 6, 2015

However different OpenGL version we can be running on support different minimum version of GLSL, so we should also be able to convert e.g. from 130 to 440, so users don't have to deal with this. However if you say we support only 120 shaders in Processing, then indeed it is not needed.

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Oct 6, 2015

Member

you man that the pre-processor shuld be able to convert between any to GLSL version? But the only purpose of the pre-preprocessing step is to make 120 shaders compatible with newer GLSL versions, right? The simplest solution is to add the #version directive in the shader code, isn't it?

Member

codeanticode commented Oct 6, 2015

you man that the pre-processor shuld be able to convert between any to GLSL version? But the only purpose of the pre-preprocessing step is to make 120 shaders compatible with newer GLSL versions, right? The simplest solution is to add the #version directive in the shader code, isn't it?

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Oct 6, 2015

Contributor

Some systems have minimum version higher than 130 so if you hardcode #version 130 it won't run on some systems. That's why it may be a good idea to just take what people wrote and upgrade it to minimum required version. Here it is done with shader preprocessor, however we don't want to depend on this extension so we should use regex.

Contributor

JakubValtar commented Oct 6, 2015

Some systems have minimum version higher than 130 so if you hardcode #version 130 it won't run on some systems. That's why it may be a good idea to just take what people wrote and upgrade it to minimum required version. Here it is done with shader preprocessor, however we don't want to depend on this extension so we should use regex.

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Oct 6, 2015

Member

Ok. I will mark it as enhancement. Let's discuss it further once you create a PR for it.

@stmaccarelli are you able to use your shaders if you add the #version directive, so no need of hacking the function names?

Member

codeanticode commented Oct 6, 2015

Ok. I will mark it as enhancement. Let's discuss it further once you create a PR for it.

@stmaccarelli are you able to use your shaders if you add the #version directive, so no need of hacking the function names?

@stmaccarelli

This comment has been minimized.

Show comment
Hide comment
@stmaccarelli

stmaccarelli Oct 6, 2015

With #version 400 it works ok.
I didn't test with #version 130, I'll do it asap.

_Stefano Maccarelli_Art director, designer, creative coder.

Via San Donato, 20 – Torino, Italy
www.stefano-maccarelli.com

www.linkedin.com/in/stmaccarelli http://www.linkedin.com/in/stmaccarelli
(+39) 340 6247854
st.maccarelli@gmail.com

With #version 400 it works ok.
I didn't test with #version 130, I'll do it asap.

_Stefano Maccarelli_Art director, designer, creative coder.

Via San Donato, 20 – Torino, Italy
www.stefano-maccarelli.com

www.linkedin.com/in/stmaccarelli http://www.linkedin.com/in/stmaccarelli
(+39) 340 6247854
st.maccarelli@gmail.com

@stmaccarelli

This comment has been minimized.

Show comment
Hide comment
@stmaccarelli

stmaccarelli Oct 6, 2015

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Oct 6, 2015

Member

@JakubValtar I tend to agree with @stmaccarelli, what you are suggesting, if I understand correctly, is that users should be able to write code for version 130, and if the driver only supports a newer version of GLSL, then the preprocessor should update the code accordingly, am I right?

Member

codeanticode commented Oct 6, 2015

@JakubValtar I tend to agree with @stmaccarelli, what you are suggesting, if I understand correctly, is that users should be able to write code for version 130, and if the driver only supports a newer version of GLSL, then the preprocessor should update the code accordingly, am I right?

@stmaccarelli

This comment has been minimized.

Show comment
Hide comment
@stmaccarelli

stmaccarelli Oct 6, 2015

Well, not exactly.
Maybe it's ok as is right now. , but can't you say in references /
javadoc that in P3 shaders declaring a #version is mandatory? A straight
approach that would make less confusion.
You could then print a warning when preprocessor runs, telling users that
a preprocessor converted #version OLD syntax to #version NEW, and suggests
to update syntax.
This way compatibility is preserved, and users know what's happening under
the hood.

_Stefano Maccarelli_Art director, designer, creative coder.

Via San Donato, 20 – Torino, Italy
www.stefano-maccarelli.com

www.linkedin.com/in/stmaccarelli http://www.linkedin.com/in/stmaccarelli
(+39) 340 6247854
st.maccarelli@gmail.com

Well, not exactly.
Maybe it's ok as is right now. , but can't you say in references /
javadoc that in P3 shaders declaring a #version is mandatory? A straight
approach that would make less confusion.
You could then print a warning when preprocessor runs, telling users that
a preprocessor converted #version OLD syntax to #version NEW, and suggests
to update syntax.
This way compatibility is preserved, and users know what's happening under
the hood.

_Stefano Maccarelli_Art director, designer, creative coder.

Via San Donato, 20 – Torino, Italy
www.stefano-maccarelli.com

www.linkedin.com/in/stmaccarelli http://www.linkedin.com/in/stmaccarelli
(+39) 340 6247854
st.maccarelli@gmail.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment