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

Various small fixes #12

Conversation

Scorbutics
Copy link

@Scorbutics Scorbutics commented Jun 20, 2024

Hello,

Following your comment posted in the issue #11, I've built a simpler list of commit that describe individually each task.

Those include:

Refactoring, not fixes:

  • common "get current program" algorithm between classic OpenGL and OpenGL ES2: 7efc4fd
  • shader compilation code is now fully common between classic OpenGL and OpenGL ES2: bf97b36

Missing Feature:

  • being able to get the ratio between actual texture size and texture size, as OpenGL ES2 only support NPOT textures: 21f4e44

Fixes:

  • some cache checks removed only for ES2: 552e98a
  • in default texture shader, there was "sf_sampler" in place of "texture": c3786f4
  • (I don't remember what problem was caused by this one) extension init moved in common code, it was done in OpenGL, also do it in OpenGL ES2: 8198317

Chores:

  • (updated .gitignore) - optional, I can remove it: 1c765e0

I've voluntary removed the c++17 code as you mentioned in the issue, to keep it compatible with c++03.
The removed code was taking out the the "sf_texture" matrix computation from RenderTarget to put it in the Texture::bind method, where it's also done for classic OpenGL.
If you are interested in doing that again (but using only c++03), do not hesitate to tell me.

I can also split this PR in several small ones, if you prefer, but it was easier for me to bundle all fixes I needed in one pull request in order to check my modifications were fine.

Unfortunately, I can't provide any minimum reproducible example code right now, as those issues were noticed while running a game with complex layers, it would required to invest some time to do it. But again, if you need it, I can do it. I just don't know when!
Also, note that building the SFML shaders official example helped me found some of those fixes, so it might be enough.

@Scorbutics Scorbutics changed the title Clean rebased gles2 2.6.x Various small fixes Jun 20, 2024
@Zombieschannel
Copy link
Owner

Looking at this now, bf97b36 this change looks good except that I'm not quite sure GL_GEOMETRY_SHADER is defined on GLES2? Since afaik geometry shaders aren't supported in GLES in any version, so that is just kind of strange.

Wanted to ask what is the purpose of this gitignore change exactly? 1c765e0

If you are interested in doing that again (but using only c++03), do not hesitate to tell me.

I wasn't really planning to change that, I'd like to keep the function parameters and return types the same as SFML 2.6.x since the main goal is for this branch to be a drop and replace for existing SFML code.

When it comes to this fix: 552e98a, I'd like to know what does it fix? Though since it is caching, I'm not sure how much it can be tested and likely the impact is small.

c3786f4 I've never really found nice default uniform names in shaders, but imo sf_sampler seemed quite solid though I'd like to hear from other people like @Nathan-M-code what they think.

I have nothing to add to these fixes: 8198317 & 7efc4fd, all good.

Just wondering, does this fix 21f4e44 actually fix this issue (or at least it adds the needed uniforms, I know shader code hasn't changed)? #13

@Nathan-M-code
Copy link

I find

"uniform sampler2D texture;"
"uniform mat4 sf_texture;"

a bit confusing. sf_sampler was better imo

@Scorbutics
Copy link
Author

Scorbutics commented Jun 30, 2024

Looking at this now, bf97b36 this change looks good except that I'm not quite sure GL_GEOMETRY_SHADER is defined on GLES2? Since afaik geometry shaders aren't supported in GLES in any version, so that is just kind of strange.

You're right, it might come from an aggressive copy paste on my side. As it's not compiled when SFML_OPENGL_ES is not defined, I didn't see it.

Wanted to ask what is the purpose of this gitignore change exactly? 1c765e0

Each time I trigger a cmake configuration, it generates a lot of noise if I don't ignore them.
Not sure why it was not ignored from the start, in the official SFML repo, but it's not related at all with OpenGL ES, so we can discard it if you want.

When it comes to this fix: 552e98a, I'd like to know what does it fix? Though since it is caching, I'm not sure how much it can be tested and likely the impact is small.

That's the tricky part... as I'm working on a full game engine port to Android, I was only able to do some tests on it.
I noticed that in my case, vertices weren't update well and disabling the cache was necessarily.
I will try to write a minimal reproducible example of code asap :)

c3786f4 I've never really found nice default uniform names in shaders, but imo sf_sampler seemed quite solid though I'd like to hear from other people like @Nathan-M-code what they think.

Oops, I thought this was badly named here but apparently texture is how we name sf_sampler in the engine I'm working on 😅. I will remove that!

Just wondering, does this fix 21f4e44 actually fix this issue (or at least it adds the needed uniforms, I know shader code hasn't changed)? #13

It's certainly related as it allows to get "virtual" texture coordinates based on size, but I'm not sure it can fix this. I might have to check that.
As an example of use:

#ifdef GL_ES
precision mediump float;

uniform mat4 sf_texture;
uniform vec2 factor_npot;

varying vec2 v_texCoord;
#else
const vec2 factor_npot = vec2(1.0, 1.0);
#endif

uniform float t;

const float PI = 3.14159265359;

void main() {
  vec4 COLORS[7];
  COLORS[0] = vec4(1.0000, 0.1490, 0.2196, 1.0);
  COLORS[1] = vec4(1.0000, 0.4196, 0.1882, 1.0);
  COLORS[2] = vec4(1.0000, 0.6353, 0.0078, 1.0);
  COLORS[3] = vec4(0.0078, 0.8157, 0.5686, 1.0);
  COLORS[4] = vec4(0.0039, 0.5020, 0.9843, 1.0);
  COLORS[5] = vec4(0.4824, 0.2118, 0.8549, 1.0);
  COLORS[6] = vec4(1.0000, 0.1490, 0.2196, 1.0);

#ifdef GL_ES
  vec2 tc = (sf_texture * vec4(v_texCoord / factor_npot, 0.0, 1.0)).xy;
#else
  vec2 tc = gl_TexCoord[0].xy;
#endif

  float circleX = mod((tc.x - t) * 6.0, 1.0);
  float colorIndex = mod((tc.x - t) * 6.0, 6.0);
  vec4 frag = mix(
    COLORS[int(floor(colorIndex))],
    COLORS[int(floor(colorIndex)) + 1],
    fract(colorIndex)
  );
  float circleRadius = pow(sin(tc.x * PI), 3.0);
  frag.a = 1.0 - (sqrt(pow((tc.y * 2.0 - 1.0) / circleRadius, 2.0) + pow(circleX * 2.0 - 1.0, 2.0)));
  gl_FragColor = frag;
}

will create this effect:
image
but without dividing by this factor, it becomes:
image
(the shader looks "cropped")

I won't be available for next two weeks so I try to build the example I mentioned quickly, otherwise I will be back for it the 16th of July.

@Scorbutics
Copy link
Author

Scorbutics commented Jun 30, 2024

Ok, for the cache commit, I got something easily reproducible. From the root SFML directory, do the following:

  • Enable building SFML examples using -DSFML_BUILD_EXAMPLES=True while configuring with cmake
  • Replace pixelate.frag and create pixelate.vert with the following sources, to make them compatible with ES2:

pixelate.frag:

precision mediump float;

uniform sampler2D texture;
uniform float pixel_threshold;
uniform mat4 sf_texture;

varying vec2 v_texCoord;
varying vec4 v_color;

void main()
{
    vec4 coord = sf_texture * vec4(v_texCoord, 0.0, 1.0);
    float factor = 1.0 / (pixel_threshold + 0.001);
    vec2 pos = floor(coord.xy * factor + 0.5) / factor;
    gl_FragColor = texture2D(texture, pos) * v_color;
}

pixelate.vert:

attribute vec2 position;
attribute vec2 texCoord;
attribute vec4 color;

uniform mat4 sf_modelview;
uniform mat4 sf_projection;

varying vec2 v_texCoord;
varying vec4 v_color;

void main() {
    v_texCoord = texCoord;
    v_color = color;
    gl_Position = sf_projection * sf_modelview * vec4(position.xy, 0.0, 1.0);
}
  • Use this pixelate.vert shader in the Shader.cpp file of examples folder (l.32):
        // Load the shader
-        if (!m_shader.loadFromFile("resources/pixelate.frag", sf::Shader::Fragment))
+        if (!m_shader.loadFromFile("resources/pixelate.vert", "resources/pixelate.frag"))
            return false;
        m_shader.setUniform("texture", sf::Shader::CurrentTexture);
  • Build and execute the shader example: cmake --build . && (cd examples/shader && ./shader)

You get the following result:
image

You can see the text box container not visible.

Then, edit RenderTarget.cpp and replace the following (l.795):

    if (useVertexCache)
    {
        // Since vertices are transformed, we must use an identity transform to render them
#ifndef SFML_OPENGL_ES
-        if (!m_cache.enable || !m_cache.useVertexCache)
+
        {
            glCheck(glLoadIdentity());

Building and executing the shader example, you can now see:

image

Better, but there is still a drawing problem...

Finally, edit RenderTarget.cpp by removing the following (l.354):

        // If we switch between non-cache and cache mode or enable texture
        // coordinates we need to set up the pointers to the vertices' components
-        if (!m_cache.enable || !useVertexCache || !m_cache.useVertexCache)
+
        {
            const char* data = reinterpret_cast<const char*>(vertices);

You'll be able to see the example fully working:
image

@Zombieschannel Zombieschannel self-assigned this Jul 9, 2024
@Zombieschannel
Copy link
Owner

All seems good! Can confirm it fixed the example with shaders. Also the factor_npot uniform (maybe not the best name but ok for now) will probably help me with this issue: #13

But yeah again, thanks a lot for these fixes!

@Zombieschannel Zombieschannel merged commit 4820f28 into Zombieschannel:SFML-2.6.x-GLES2 Jul 9, 2024
11 of 23 checks passed
@Scorbutics
Copy link
Author

With pleasure. If by any means I find something else (I did not run all shader examples), I will tell you

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