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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenGL improvements #779

Merged
merged 12 commits into from Mar 26, 2015

Conversation

Projects
None yet
@binary1248
Member

binary1248 commented Jan 13, 2015

It's done 馃榿.

I was too lazy to copy all the commit messages here, so if you have any questions feel free to ask.

Needs testing on OS X.

@binary1248 binary1248 added this to the 2.3 milestone Jan 13, 2015

@binary1248 binary1248 self-assigned this Jan 13, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 13, 2015

Member

Yeah 馃憤

Wouldn't it be clearer to add an enum for the context flags, rather than separate booleans? It would take less arguments, and be clearer if set with the constructor.

Isn't there a license associated to the code generated by GLLoader?

Was it really necessary to request a 24-bit depth buffer in the OpenGL and Window examples? The code is supposed to pick the closest valid configuration if some attributes are not available.

Member

LaurentGomila commented Jan 13, 2015

Yeah 馃憤

Wouldn't it be clearer to add an enum for the context flags, rather than separate booleans? It would take less arguments, and be clearer if set with the constructor.

Isn't there a license associated to the code generated by GLLoader?

Was it really necessary to request a 24-bit depth buffer in the OpenGL and Window examples? The code is supposed to pick the closest valid configuration if some attributes are not available.

#define castToGlHandle(x) (x)
#define castFromGlHandle(x) (x)
#endif

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Jan 13, 2015

Member

Why do you use macros here?

@eXpl0it3r

eXpl0it3r Jan 13, 2015

Member

Why do you use macros here?

This comment has been minimized.

@binary1248

binary1248 Jan 13, 2015

Member

Because those lines are literally no-ops. I've never seen any cases where one declares functions just to conditionally cast stuff. Then again not many people run into this problem that often either. 馃槢

What do you suggest?

@binary1248

binary1248 Jan 13, 2015

Member

Because those lines are literally no-ops. I've never seen any cases where one declares functions just to conditionally cast stuff. Then again not many people run into this problem that often either. 馃槢

What do you suggest?

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Jan 13, 2015

Member

Ah okay, yeah I guess it makes sense. 馃槈

@eXpl0it3r

eXpl0it3r Jan 13, 2015

Member

Ah okay, yeah I guess it makes sense. 馃槈

This comment has been minimized.

@TankOs

TankOs Jan 13, 2015

Member

I suggest a function. Is there any issue with a function?

@TankOs

TankOs Jan 13, 2015

Member

I suggest a function. Is there any issue with a function?

This comment has been minimized.

@binary1248

binary1248 Jan 13, 2015

Member

What would the function look like?

@binary1248

binary1248 Jan 13, 2015

Member

What would the function look like?

This comment has been minimized.

@binary1248

binary1248 Jan 13, 2015

Member

It does. Since T is unsigned int, the following happens:

Tank provided an example for castFromGlHandle() but named it wrong and had the wrong return type (should be unsigned int).

You guys forget that this is the castToGlHandle() function. 馃槈 It has to cast from a fixed type (unsigned int) in both cases to whatever GLEXT_GLhandle is. castFromGlHandle() casts from a GLEXT_GLhandle to unsigned int in both cases. So yeah... your template solution only works for one direction but not the other.

Actually, do you even need the case differentiation at all? Can't you just use reinterpret_cast() in all cases, even if input and output are identical? Never tried that to be honest, but don't see why it shouldn't work.

reinterpret_cast() doesn't allow casting from a pointer to an integral value when the conversion would truncate the data (i.e. 64-bit pointer -> 32-bit unsigned int). I tried this already. Apple really tried their hardest to be as annoying as possible. 馃槈 But yeah... I guess the inner static_cast in the "To" case can be removed. But moving the double cast into every line where it is needed would make the code really horrible to read IMHO.

I'm still interested to see suggestions for how to cast from an unsigned int to GLEXT_GLhandle that works in both Apple and non-Apple cases 馃榿.

@binary1248

binary1248 Jan 13, 2015

Member

It does. Since T is unsigned int, the following happens:

Tank provided an example for castFromGlHandle() but named it wrong and had the wrong return type (should be unsigned int).

You guys forget that this is the castToGlHandle() function. 馃槈 It has to cast from a fixed type (unsigned int) in both cases to whatever GLEXT_GLhandle is. castFromGlHandle() casts from a GLEXT_GLhandle to unsigned int in both cases. So yeah... your template solution only works for one direction but not the other.

Actually, do you even need the case differentiation at all? Can't you just use reinterpret_cast() in all cases, even if input and output are identical? Never tried that to be honest, but don't see why it shouldn't work.

reinterpret_cast() doesn't allow casting from a pointer to an integral value when the conversion would truncate the data (i.e. 64-bit pointer -> 32-bit unsigned int). I tried this already. Apple really tried their hardest to be as annoying as possible. 馃槈 But yeah... I guess the inner static_cast in the "To" case can be removed. But moving the double cast into every line where it is needed would make the code really horrible to read IMHO.

I'm still interested to see suggestions for how to cast from an unsigned int to GLEXT_GLhandle that works in both Apple and non-Apple cases 馃榿.

This comment has been minimized.

@TankOs

TankOs Jan 14, 2015

Member

https://gist.github.com/TankOs/d89f0d58fe48367bb5bd

Maybe you can even get rid of the still remaining PP directive, haven't checked the original code.

@TankOs

TankOs Jan 14, 2015

Member

https://gist.github.com/TankOs/d89f0d58fe48367bb5bd

Maybe you can even get rid of the still remaining PP directive, haven't checked the original code.

This comment has been minimized.

@binary1248

binary1248 Jan 14, 2015

Member

馃榿

Like I said, castToGlHandle has to have the signature

GLEXT_GLhandle castToGlHandle(unsigned int)

in both cases. Your gist still doesn't do this. Those macros aren't for casting from void* to anything else or back. They are only there to cast from unsigned int to GLEXT_GLhandle and vice versa. It's the GLEXT_GLhandle type that changes and not the function signature.

In essence, you would have to have 2 functions both with the same name and taking the same parameter type but returning a different type depending on system. Like I said, I don't think this is doable with templates alone.

@binary1248

binary1248 Jan 14, 2015

Member

馃榿

Like I said, castToGlHandle has to have the signature

GLEXT_GLhandle castToGlHandle(unsigned int)

in both cases. Your gist still doesn't do this. Those macros aren't for casting from void* to anything else or back. They are only there to cast from unsigned int to GLEXT_GLhandle and vice versa. It's the GLEXT_GLhandle type that changes and not the function signature.

In essence, you would have to have 2 functions both with the same name and taking the same parameter type but returning a different type depending on system. Like I said, I don't think this is doable with templates alone.

This comment has been minimized.

@LaurentGomila

LaurentGomila Jan 14, 2015

Member

I vote for the macros. We're wasting so much time overengineering this, just for the beauty of C++... Let's be pragmatic and try to make more useful comments on this huge PR 馃槢

@LaurentGomila

LaurentGomila Jan 14, 2015

Member

I vote for the macros. We're wasting so much time overengineering this, just for the beauty of C++... Let's be pragmatic and try to make more useful comments on this huge PR 馃槢

This comment has been minimized.

@TankOs

TankOs Jan 14, 2015

Member

Ahhhh, got it now. Macro seems legit then. 馃樇

@TankOs

TankOs Jan 14, 2015

Member

Ahhhh, got it now. Macro seems legit then. 馃樇

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 13, 2015

Member

Wouldn't it be clearer to add an enum for the context flags, rather than separate booleans? It would take less arguments, and be clearer if set with the constructor.

Perhaps... didn't think of that all this time somehow 馃槢.

Isn't there a license associated to the code generated by GLLoader?

No... we use a loader generator to generate the code that you see in GLLoader.cpp and GLLoader.hpp, and even that (initial) code is heavily modified. It's like using MS Paint to draw a picture and asking if that picture has to have a specific license 馃槢. I don't see any problems here.

Was it really necessary to request a 24-bit depth buffer in the OpenGL and Window examples? The code is supposed to pick the closest valid configuration if some attributes are not available.

If you try to request settings that don't perfectly match what the system can provide, you will stumble into the fallback path. On some systems, that might lead to other settings not being met as well, going as far as decreasing the OpenGL version number to < 3.0 because the initial attempt at getting a 3.0+ didn't yield any matching configurations. Sure, it isn't that important in the examples, but I figured since they are examples, why not make them able to run on more systems without having to go into the fall back path 馃槢.

There currently isn't a way for the user to enumerate all the valid configurations that the system can provide, so if a certain setting is important to them, they just have to keep trying all possible combinations until nothing gets "downgraded". This happens even when settings are completely unrelated. This might be something we would want to add in the future in addition to the video mode enumeration.

Member

binary1248 commented Jan 13, 2015

Wouldn't it be clearer to add an enum for the context flags, rather than separate booleans? It would take less arguments, and be clearer if set with the constructor.

Perhaps... didn't think of that all this time somehow 馃槢.

Isn't there a license associated to the code generated by GLLoader?

No... we use a loader generator to generate the code that you see in GLLoader.cpp and GLLoader.hpp, and even that (initial) code is heavily modified. It's like using MS Paint to draw a picture and asking if that picture has to have a specific license 馃槢. I don't see any problems here.

Was it really necessary to request a 24-bit depth buffer in the OpenGL and Window examples? The code is supposed to pick the closest valid configuration if some attributes are not available.

If you try to request settings that don't perfectly match what the system can provide, you will stumble into the fallback path. On some systems, that might lead to other settings not being met as well, going as far as decreasing the OpenGL version number to < 3.0 because the initial attempt at getting a 3.0+ didn't yield any matching configurations. Sure, it isn't that important in the examples, but I figured since they are examples, why not make them able to run on more systems without having to go into the fall back path 馃槢.

There currently isn't a way for the user to enumerate all the valid configurations that the system can provide, so if a certain setting is important to them, they just have to keep trying all possible combinations until nothing gets "downgraded". This happens even when settings are completely unrelated. This might be something we would want to add in the future in addition to the video mode enumeration.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 14, 2015

Member

Needs testing on OS X.

Alright. I'll do it probably in 10 days (after the exams). Beside code snippet from #778 and the examples, is there anything specific I should test? Any corner cases?

Member

mantognini commented Jan 14, 2015

Needs testing on OS X.

Alright. I'll do it probably in 10 days (after the exams). Beside code snippet from #778 and the examples, is there anything specific I should test? Any corner cases?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 14, 2015

Member

#778 occurred on Windows, or at least I encountered it only on Windows. It might not be an issue on OS X even with the current master.

As for testing the rest, no there isn't anything specific to test. If the pong, shader and OpenGL examples build and run fine (without any warnings as well) then it is more or less safe to assume that it "just works" on OS X.

Member

binary1248 commented Jan 14, 2015

#778 occurred on Windows, or at least I encountered it only on Windows. It might not be an issue on OS X even with the current master.

As for testing the rest, no there isn't anything specific to test. If the pong, shader and OpenGL examples build and run fine (without any warnings as well) then it is more or less safe to assume that it "just works" on OS X.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 14, 2015

Member

馃憣 I'll test that.

Member

mantognini commented Jan 14, 2015

馃憣 I'll test that.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 20, 2015

Member

The new version provides the context attribute flags as enums.

Anything else? 馃榿

Member

binary1248 commented Jan 20, 2015

The new version provides the context attribute flags as enums.

Anything else? 馃榿

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 20, 2015

Member

Flags that can be combined use the Uint32 type in other classes (sf::Window, sf::Text), so... 馃槉

Member

LaurentGomila commented Jan 20, 2015

Flags that can be combined use the Uint32 type in other classes (sf::Window, sf::Text), so... 馃槉

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 20, 2015

Member

Anything else? 馃榿

Member

binary1248 commented Jan 20, 2015

Anything else? 馃榿

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 21, 2015

Member

As for testing the rest, no there isn't anything specific to test. If the pong, shader and OpenGL examples build and run fine (without any warnings as well) then it is more or less safe to assume that it "just works" on OS X.

Pong and shader run fine. However, the OpenGL sample is broken: there is no longer the cube in place of the cursor. Is it the same on Linux/Windows or yet another OS X speciality?

Member

mantognini commented Jan 21, 2015

As for testing the rest, no there isn't anything specific to test. If the pong, shader and OpenGL examples build and run fine (without any warnings as well) then it is more or less safe to assume that it "just works" on OS X.

Pong and shader run fine. However, the OpenGL sample is broken: there is no longer the cube in place of the cursor. Is it the same on Linux/Windows or yet another OS X speciality?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 21, 2015

Member

I always test all examples on all the systems I have (Windows and Linux). The OpenGL example works on both for me. If you can, try to get a console up and look for errors. It might have something to do with the context settings/versions.

Member

binary1248 commented Jan 21, 2015

I always test all examples on all the systems I have (Windows and Linux). The OpenGL example works on both for me. If you can, try to get a console up and look for errors. It might have something to do with the context settings/versions.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 21, 2015

Member

Nothing in the terminal nor the logger. Using git bitsect I can incriminate 1dae59c, if that helps you...

Member

mantognini commented Jan 21, 2015

Nothing in the terminal nor the logger. Using git bitsect I can incriminate 1dae59c, if that helps you...

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 21, 2015

Member

The "strange" thing is that according to your description, only the rotating cube is missing. Since the background and text are still rendered properly, sfml-graphics and sfml-window have to work as intended in regards to how they take care of context management. The problem has to be specific to the OpenGL code (or build configuration settings?) in the example since it doesn't occur with the the pong or shader examples.

Member

binary1248 commented Jan 21, 2015

The "strange" thing is that according to your description, only the rotating cube is missing. Since the background and text are still rendered properly, sfml-graphics and sfml-window have to work as intended in regards to how they take care of context management. The problem has to be specific to the OpenGL code (or build configuration settings?) in the example since it doesn't occur with the the pong or shader examples.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 22, 2015

Member

I've to admit I don't understand everything that's going on. For example, I see Unix/GlxExtensions.* and Win32/WglExtensions.* but nothing similar for the other platforms, is it intentional?

I've enable more warnings at compilation, I get a lot of this:

SFML/src/SFML/Graphics/GLLoader.cpp:56:34: warning: cast between
      pointer-to-function and pointer-to-object is an extension [-Wpedantic]
    sf_ptrc_glBlendEquationEXT = (void (CODEGEN_FUNCPTR *)(GLenum))IntGetProcAddress("glBlendEquationEXT");
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

No idea if it matters, though.

BTW, I also get this one, not that it really matters.

SFML/include/SFML/Window/ContextSettings.hpp:46:25: warning: commas at the end of
      enumerator lists are a C++11 extension [-Wc++11-extensions]
        Debug   = 1 << 2, ///< Debug attribute

Regarding those two flags (Debug and Core), the doc should probably state that they do nothing on OS X.

Member

mantognini commented Jan 22, 2015

I've to admit I don't understand everything that's going on. For example, I see Unix/GlxExtensions.* and Win32/WglExtensions.* but nothing similar for the other platforms, is it intentional?

I've enable more warnings at compilation, I get a lot of this:

SFML/src/SFML/Graphics/GLLoader.cpp:56:34: warning: cast between
      pointer-to-function and pointer-to-object is an extension [-Wpedantic]
    sf_ptrc_glBlendEquationEXT = (void (CODEGEN_FUNCPTR *)(GLenum))IntGetProcAddress("glBlendEquationEXT");
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

No idea if it matters, though.

BTW, I also get this one, not that it really matters.

SFML/include/SFML/Window/ContextSettings.hpp:46:25: warning: commas at the end of
      enumerator lists are a C++11 extension [-Wc++11-extensions]
        Debug   = 1 << 2, ///< Debug attribute

Regarding those two flags (Debug and Core), the doc should probably state that they do nothing on OS X.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 22, 2015

Member

For example, I see Unix/GlxExtensions.* and Win32/WglExtensions.* but nothing similar for the other platforms, is it intentional?

Yes... OS X doesn't rely on an extension to get access to the newer context management functions.

I'll look into those warnings. They shouldn't really change anything though, since pong and shader run fine. You do see the background and text in the OpenGL example right? Just not the spinning cube? Also, does it stop working right after the commit that introduces GLLoader, or only after a later commit?

Member

binary1248 commented Jan 22, 2015

For example, I see Unix/GlxExtensions.* and Win32/WglExtensions.* but nothing similar for the other platforms, is it intentional?

Yes... OS X doesn't rely on an extension to get access to the newer context management functions.

I'll look into those warnings. They shouldn't really change anything though, since pong and shader run fine. You do see the background and text in the OpenGL example right? Just not the spinning cube? Also, does it stop working right after the commit that introduces GLLoader, or only after a later commit?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 23, 2015

Member

You do see the background and text in the OpenGL example right? Just not the spinning cube?

Exactly, I see only background and text.

Also, does it stop working right after the commit that introduces GLLoader, or only after a later commit?

Commit on bugfix/glx_context are fine, the first with this issue is the one introducing the loader, that is, after your rebase, this one: dab8ecc

Member

mantognini commented Jan 23, 2015

You do see the background and text in the OpenGL example right? Just not the spinning cube?

Exactly, I see only background and text.

Also, does it stop working right after the commit that introduces GLLoader, or only after a later commit?

Commit on bugfix/glx_context are fine, the first with this issue is the one introducing the loader, that is, after your rebase, this one: dab8ecc

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 28, 2015

Member

Hmm... I'm running out of ideas 馃槙.

Can you try and replace the rotating cube by something "simpler", like a simple untextured triangle using glBegin() and glEnd()? It might be one of those cases where the code relies on... "not so well specified behaviour" such as the starting OpenGL state which might be different on different implementations.

Also... it would be nice if others with OS X can test as well to see if this is "reproducible" on their systems as well.

Member

binary1248 commented Jan 28, 2015

Hmm... I'm running out of ideas 馃槙.

Can you try and replace the rotating cube by something "simpler", like a simple untextured triangle using glBegin() and glEnd()? It might be one of those cases where the code relies on... "not so well specified behaviour" such as the starting OpenGL state which might be different on different implementations.

Also... it would be nice if others with OS X can test as well to see if this is "reproducible" on their systems as well.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jan 28, 2015

Member

I tried a few things, such as

        glColor3f(1.0, 1.0, 1.0);
        glMatrixMode(GL_PROJECTION);
        glLoadIdentity();
        glOrtho(0.0, 1.0, 0.0, 1.0, -1.0, 1.0);
        glBegin(GL_POLYGON);
        glVertex3f(0.25, 0.25, 1.0);
        glVertex3f(0.75, 0.25, 1.0);
        glVertex3f(0.75, 0.75, 0.0);
        glVertex3f(0.25, 0.75, 0.0);
        glEnd();

which works fine.

And by luck, I ended up having something close to working. In this gist you can find a edited version of the OpenGL example where basically I moved all ogl code inside the main loop (except for the texture loading). Here is the result:

ugly

Instead of no cube I got this brownish thing...

Does it give you any idea of where I should look next?

Member

mantognini commented Jan 28, 2015

I tried a few things, such as

        glColor3f(1.0, 1.0, 1.0);
        glMatrixMode(GL_PROJECTION);
        glLoadIdentity();
        glOrtho(0.0, 1.0, 0.0, 1.0, -1.0, 1.0);
        glBegin(GL_POLYGON);
        glVertex3f(0.25, 0.25, 1.0);
        glVertex3f(0.75, 0.25, 1.0);
        glVertex3f(0.75, 0.75, 0.0);
        glVertex3f(0.25, 0.75, 0.0);
        glEnd();

which works fine.

And by luck, I ended up having something close to working. In this gist you can find a edited version of the OpenGL example where basically I moved all ogl code inside the main loop (except for the texture loading). Here is the result:

ugly

Instead of no cube I got this brownish thing...

Does it give you any idea of where I should look next?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 29, 2015

Member

Hmm... somehow the texture isn't being applied. What happens if you move the texture creation block inside the loop (preferably after enabling GL_TEXTURE_2D) as well? You will want to delete the texture object at the end of the loop body if you do this...

Member

binary1248 commented Jan 29, 2015

Hmm... somehow the texture isn't being applied. What happens if you move the texture creation block inside the loop (preferably after enabling GL_TEXTURE_2D) as well? You will want to delete the texture object at the end of the loop body if you do this...

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 4, 2015

Member

鈽濓笍 馃榿

Member

binary1248 commented Feb 4, 2015

鈽濓笍 馃榿

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 5, 2015

Member

Sorry, I didn't see your previous message...

So, just moving L50-L60 inside the main loop doesn't improve things. Now I remembered you mentioning resetGLStates() on the forum recently. So I tried adding window.resetGLStates(); right before loading (see this other gist) and it worked. :-)

I tried to do the same and add window.resetGLStates(); before line 50 in the first gist and it worked too.

If I do it on line 49 of the opengl example, it works as well!

Do you have any clue why it behaves like that?

Member

mantognini commented Feb 5, 2015

Sorry, I didn't see your previous message...

So, just moving L50-L60 inside the main loop doesn't improve things. Now I remembered you mentioning resetGLStates() on the forum recently. So I tried adding window.resetGLStates(); right before loading (see this other gist) and it worked. :-)

I tried to do the same and add window.resetGLStates(); before line 50 in the first gist and it worked too.

If I do it on line 49 of the opengl example, it works as well!

Do you have any clue why it behaves like that?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 5, 2015

Member

Well... the next step would be to replace window.resetGLStates(); with the code that it runs 馃槢.

Place the following block where window.resetGLStates(); is (line 49 of the opengl example) and see if it works.

sf::Shader::isAvailable();
window.setActive();

glClientActiveTexture(GL_TEXTURE0);
glActiveTexture(GL_TEXTURE0);

glDisable(GL_CULL_FACE);
glDisable(GL_LIGHTING);
glDisable(GL_DEPTH_TEST);
glDisable(GL_ALPHA_TEST);
glEnable(GL_TEXTURE_2D);
glEnable(GL_BLEND);
glMatrixMode(GL_MODELVIEW);
glEnableClientState(GL_VERTEX_ARRAY);
glEnableClientState(GL_COLOR_ARRAY);
glEnableClientState(GL_TEXTURE_COORD_ARRAY);

glBlendFuncSeparate(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA, GL_ONE, GL_ONE_MINUS_SRC_ALPHA);
glBlendEquationSeparate(GL_FUNC_ADD, GL_FUNC_ADD);
glLoadIdentity();
sf::Texture::bind(NULL, sf::Texture::Pixels);
sf::Shader::bind(NULL);

window.setView(window.getView());

If it does work, try to remove 1 line at a time until it doesn't work any longer. Hopefully we will end up with 1 or 2 lines that are responsible for "fixing" the problem.

Member

binary1248 commented Feb 5, 2015

Well... the next step would be to replace window.resetGLStates(); with the code that it runs 馃槢.

Place the following block where window.resetGLStates(); is (line 49 of the opengl example) and see if it works.

sf::Shader::isAvailable();
window.setActive();

glClientActiveTexture(GL_TEXTURE0);
glActiveTexture(GL_TEXTURE0);

glDisable(GL_CULL_FACE);
glDisable(GL_LIGHTING);
glDisable(GL_DEPTH_TEST);
glDisable(GL_ALPHA_TEST);
glEnable(GL_TEXTURE_2D);
glEnable(GL_BLEND);
glMatrixMode(GL_MODELVIEW);
glEnableClientState(GL_VERTEX_ARRAY);
glEnableClientState(GL_COLOR_ARRAY);
glEnableClientState(GL_TEXTURE_COORD_ARRAY);

glBlendFuncSeparate(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA, GL_ONE, GL_ONE_MINUS_SRC_ALPHA);
glBlendEquationSeparate(GL_FUNC_ADD, GL_FUNC_ADD);
glLoadIdentity();
sf::Texture::bind(NULL, sf::Texture::Pixels);
sf::Shader::bind(NULL);

window.setView(window.getView());

If it does work, try to remove 1 line at a time until it doesn't work any longer. Hopefully we will end up with 1 or 2 lines that are responsible for "fixing" the problem.

@binary1248 binary1248 referenced this pull request Feb 6, 2015

Closed

Fix GLX context creation #768

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 6, 2015

Member

I'll just jump in to say, that the OpenGL example works fine for me on Windows 8.1 with my AMD Radeon HD 7500M/7600M.

Member

eXpl0it3r commented Feb 6, 2015

I'll just jump in to say, that the OpenGL example works fine for me on Windows 8.1 with my AMD Radeon HD 7500M/7600M.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 11, 2015

Member

So, apparently, the sf::Shader::isAvailable + window.setActive duo workarounds the issue. But when I tried to replace isAvailable by its code that has side effects, I couldn't make the texture work.

Do you see any reason why, in this specific piece of code, sf::Shader::isAvailable is not equal to the next block?

{
    sf::Context ctx;
    sf::priv::ensureExtensionsInit();
}

(full code)

Member

mantognini commented Feb 11, 2015

So, apparently, the sf::Shader::isAvailable + window.setActive duo workarounds the issue. But when I tried to replace isAvailable by its code that has side effects, I couldn't make the texture work.

Do you see any reason why, in this specific piece of code, sf::Shader::isAvailable is not equal to the next block?

{
    sf::Context ctx;
    sf::priv::ensureExtensionsInit();
}

(full code)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 9, 2015

Member

Needs rebasing.

@MarioLiebisch Could you give this a try on Android?

Member

eXpl0it3r commented Mar 9, 2015

Needs rebasing.

@MarioLiebisch Could you give this a try on Android?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 9, 2015

Member

Compiles, links and runs fine on my Nexus 5 as far as I can tell.

Member

MarioLiebisch commented Mar 9, 2015

Compiles, links and runs fine on my Nexus 5 as far as I can tell.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Mar 9, 2015

Member

Just one minor note: there is still old copyright years and Laurent's email address in the added header files.

Member

zsbzsb commented Mar 9, 2015

Just one minor note: there is still old copyright years and Laurent's email address in the added header files.

binary1248 added some commits Jan 8, 2015

Replaced GLEW with (a highly customized) loader generated by glLoadGe鈥
鈥, restructured GLExtensions.hpp for easier extension bookkeeping, make use of GLEXT definitions in Shader.cpp and Texture.cpp as well, replaced GL_MAX_TEXTURE_COORDS with GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS, implemented flags for requesting a debug or core/compatibility profile context, changed the default context version from 2.0 to 2.1.
Added MESA and SGI swap interval implementations in order to fix v-sy鈥
鈥c not being set properly on some Unix systems (#727), added error message when setting v-sync fails on Windows systems.
Adjusted OpenGL and Window example to request a 24-bit instead of a 3鈥
鈥2-bit depth buffer since it might not be supported on all systems.
Added check to context creation to warn the user of an incompatible m鈥
鈥smatch between the context they requested and the context that was created.
Fixed requesting an unsupported OpenGL context version causing X to c鈥
鈥ose the application, fixed GlContext initialization not updating settings properly, added error checks to GLLoader.cpp and fixed GL errors occurring when using a >= 3.0 OpenGL context.
Removed separate GLXFBConfig selection during context creation (it is鈥
鈥 chosen to match the window's already selected visual), reverted to conservative context creation only using glXCreateContextAttribsARB when absolutely necessary.
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 23, 2015

Member

Rebased onto master and updated copyright notices. I think with the exception of #803 (which - pending further information - I have to assume is caused by the Optimus setup involved), there should be no more issues with this branch.

Feel free to give it a final test run and code review. It's time to put this branch behind us. 馃槈

Member

binary1248 commented Mar 23, 2015

Rebased onto master and updated copyright notices. I think with the exception of #803 (which - pending further information - I have to assume is caused by the Optimus setup involved), there should be no more issues with this branch.

Feel free to give it a final test run and code review. It's time to put this branch behind us. 馃槈

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 23, 2015

Member

This PR has been added to my merge list, meaning it will be merged after a few positive feedbacks, unless someone raises any concerns.

Member

eXpl0it3r commented Mar 23, 2015

This PR has been added to my merge list, meaning it will be merged after a few positive feedbacks, unless someone raises any concerns.

@zsbzsb zsbzsb referenced this pull request Mar 23, 2015

Closed

Track releasing 2.3 #70

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 23, 2015

Member

Feel free to give it a final test run and code review.

Still working fine here. :-)

Member

mantognini commented Mar 23, 2015

Feel free to give it a final test run and code review.

Still working fine here. :-)

@eXpl0it3r eXpl0it3r merged commit cee6263 into master Mar 26, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 26, 2015

Member

馃帀 馃帀

Member

eXpl0it3r commented Mar 26, 2015

馃帀 馃帀

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 26, 2015

Member

馃憦 馃憦 馃槂

Member

mantognini commented Mar 26, 2015

馃憦 馃憦 馃槂

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 26, 2015

Contributor

Quick question: Why has Shader::getNativeHandle() been implemented twice?

Contributor

Foaly commented on src/SFML/Graphics/Shader.cpp in c174868 Mar 26, 2015

Quick question: Why has Shader::getNativeHandle() been implemented twice?

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 26, 2015

Member

#else // SFML_OPENGL_ES

Member

binary1248 replied Mar 26, 2015

#else // SFML_OPENGL_ES

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Mar 26, 2015

Contributor

Ah I didn't see that! Makes sense!

Contributor

Foaly replied Mar 26, 2015

Ah I didn't see that! Makes sense!

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 4, 2015

Member

On my Windows system, this error message is always printed, even if the actually created context is better (because the core profile seems to be used). This is rather annoying... Shouldn't it just warn if a worse context is found? Why does it even select a core context in the first place?

Warning: The created OpenGL context does not fully meet the settings that were requested
Requested: version = 2.1 ; depth bits = 0 ; stencil bits = 0 ; AA level = 0 ; core = false ; debug = false
Created: version = 3.1 ; depth bits = 24 ; stencil bits = 8 ; AA level = 0 ; core = true ; debug = false

Member

Bromeon commented on 1d16748 Apr 4, 2015

On my Windows system, this error message is always printed, even if the actually created context is better (because the core profile seems to be used). This is rather annoying... Shouldn't it just warn if a worse context is found? Why does it even select a core context in the first place?

Warning: The created OpenGL context does not fully meet the settings that were requested
Requested: version = 2.1 ; depth bits = 0 ; stencil bits = 0 ; AA level = 0 ; core = false ; debug = false
Created: version = 3.1 ; depth bits = 24 ; stencil bits = 8 ; AA level = 0 ; core = true ; debug = false

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 4, 2015

Member

The check has a purpose: preventing anomalous behaviour due to context incompatibilities. Getting back a core context when you request a non-core i.e. compatibility context is a problem for sfml-graphics, since it will not be able to function.

The real problem in your case seems to be that 3.1 is the highest version that your driver supports, and that your driver supports the GL_ARB_compatibility extension which SFML doesn't detect and take into account (yet). Without that extension present, rendering would be completely broken for you and you would wonder why nothing works. That's where this warning comes in.

I'll fix up the check to take GL_ARB_compatibility into account, then all should be good again. In the meantime, you can check if GL_ARB_compatibility support is present on your system, that would be the only explanation I have for the current behaviour.

Member

binary1248 replied Apr 4, 2015

The check has a purpose: preventing anomalous behaviour due to context incompatibilities. Getting back a core context when you request a non-core i.e. compatibility context is a problem for sfml-graphics, since it will not be able to function.

The real problem in your case seems to be that 3.1 is the highest version that your driver supports, and that your driver supports the GL_ARB_compatibility extension which SFML doesn't detect and take into account (yet). Without that extension present, rendering would be completely broken for you and you would wonder why nothing works. That's where this warning comes in.

I'll fix up the check to take GL_ARB_compatibility into account, then all should be good again. In the meantime, you can check if GL_ARB_compatibility support is present on your system, that would be the only explanation I have for the current behaviour.

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 4, 2015

Member

In the meantime, you can check if GL_ARB_compatibility support is present on your system, that would be the only explanation I have for the current behaviour.

When I check with glGetString(GL_EXTENSIONS), GL_ARB_compatibility appears in the list, so it should be supported.

(Strangely, OpenGL extension viewer marks the extension with a gray instead of green bullet, no idea what this means...)

Member

Bromeon replied Apr 4, 2015

In the meantime, you can check if GL_ARB_compatibility support is present on your system, that would be the only explanation I have for the current behaviour.

When I check with glGetString(GL_EXTENSIONS), GL_ARB_compatibility appears in the list, so it should be supported.

(Strangely, OpenGL extension viewer marks the extension with a gray instead of green bullet, no idea what this means...)

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 4, 2015

Member

Should be fixed in #859.

Member

binary1248 replied Apr 4, 2015

Should be fixed in #859.

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