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

[staging] glsl: Initial GLSL ES 3.x support. #65

Merged
merged 2 commits into from
Nov 11, 2023

Conversation

JohnnyonFlame
Copy link
Contributor

Started discussion on issue #64 but ended up talking with @flibitijibibo over discord who told me to open a PR and wait for a review.

Thanks again!

@flibitijibibo flibitijibibo self-assigned this Oct 28, 2023
Copy link
Collaborator

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

Most of this seems pretty straightforward, just a few tiny bits here and there. Everything else lgtm.

@@ -485,7 +485,7 @@ static void prepend_glsl_texlod_extensions(Context *ctx)
// so we'll use them if available. Failing that, we'll just fallback
// to a regular texture2D call and hope the mipmap it chooses is close
// enough.
if (!ctx->glsl_generated_texlod_setup)
if (!ctx->glsl_generated_texlod_setup && !support_glsles3(ctx))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this something we should worry about for version 100 as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fine for 100, I'm skipping this due to the syntax being deprecated for GLSL ES 3.00.

@@ -855,12 +882,20 @@ void emit_GLSL_attribute(Context *ctx, RegisterType regtype, int regnum,
int flags)
{
// !!! FIXME: this function doesn't deal with write masks at all yet!
const char *qualifier_in = "varying";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be attribute?

Copy link
Contributor Author

@JohnnyonFlame JohnnyonFlame Oct 30, 2023

Choose a reason for hiding this comment

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

Sorta - varying for fragment, attribute for vertex. It's indeed a silly mistake tho, fixed.

usage_str = "gl_FragColor"; // maybe faster?
if (support_glsles3(ctx))
{
usage_str = "_gl_FragData";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might need a quick comment saying why ES3 needs a different path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

            // gl_FragColor/FragData were deprecated in favor of manually
            // defining your own output variables, GLSL ES 3.x enforces the new
            // syntax, so let's use it.

@JohnnyonFlame
Copy link
Contributor Author

Ooops, sorry, force pushed because something got staged by mistake.

Copy link
Collaborator

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

Latest works for me - will likely merge this right after the 23.11 tag, so no rush on this one!

@flibitijibibo flibitijibibo merged commit 2428453 into icculus:main Nov 11, 2023
3 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.

2 participants