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

ALLEGRO_OPENGL_MAJOR_VERSION and ALLEGRO_OPENGL_MINOR_VERSION do not work #948

Open
bigov opened this Issue Oct 22, 2018 · 27 comments

Comments

Projects
None yet
5 participants
@bigov
Copy link

bigov commented Oct 22, 2018

The function al_create_display() set OpenGL context only witn 3.0 version, or the most current version. Parameters ALLEGRO_OPENGL_MAJOR_VERSION and ALLEGRO_OPENGL_MINOR_VERSION do not work.
It has to do with the _al_get_suggested_display_option function possibly returning the wrong values. The problem is discussed on allegro.cc

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 23, 2018

There are 2 bugs that I have found. One is expecting the value (1 << option) where option is 33 or 34 to have meaning. The other is that glGetString(GL_EXTENSIONS) is returning null.
https://www.allegro.cc/forums/thread/617591/1039695#target

@dos1

This comment has been minimized.

Copy link
Contributor

dos1 commented Oct 23, 2018

Correct me if I'm wrong, but I believe that while you can request any version for the context, the created context just has to be compatible with what you request and the driver is free to return a context that identifies itself as any other version.

Also, remember that with recent OpenGL there are two profiles - Core and Compatibility - and some drivers support different subsets of OpenGL versions for those profiles (for instance, Mesa is pretty much limited to 3.1 with Compatibility profile, which is what Allegro is using by default).

@bigov

This comment has been minimized.

Copy link
Author

bigov commented Oct 23, 2018

This does not change anything. In this place, we simply try to help developers locate a possible bug in the library's code.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 24, 2018

I have successfully requested and created a 3.2 context. But there are further problems with allegro's _al_glGetStringi function pointer. It is null, so neither print_extensions((const char*)glGetString(GL_EXTENSIONS)) nor print_extensions_3_0 using glGetStringi works.

https://www.allegro.cc/forums/thread/617591/1039711#target

@dos1

This comment has been minimized.

Copy link
Contributor

dos1 commented Oct 25, 2018

Well, of course it's null there - the comment that's already there even says so.

Depending on glGetString in this place is one of the many things that make Allegro depend on Compatibility Profile contexts. I already looked into it some months ago, but haven't spend more time on it yet (I will have to at some point though).

I'm still not sure what exactly is the problem that's being discussed here.

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Oct 25, 2018

From what I can tell it seems like @bigov expects to always get the exact version of OpenGL requested, rather than “requested or any later version”.

@dos1

This comment has been minimized.

Copy link
Contributor

dos1 commented Oct 25, 2018

I'm not sure Allegro can do that at all.

From WGL spec:

The attribute names WGL_CONTEXT_MAJOR_VERSION_ARB and WGL_CONTEXT_MINOR_VERSION_ARB request an OpenGL context supporting the specified version of the API. If successful, the context returned must be backwards compatible with the context requested.

AFAIR it was the same for GLX, and it's likely the same in NSOpenGL and EGL as well.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 25, 2018

I'm telling you, there is a bug. Before, the branch that would have asked for an explicit version would never be visisted. It would always return the newest context available. I'm in the middle of fixing it so that _al_ogl_manage_extensions doesn't crash.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 25, 2018

Here is a patch to fix the problem with glGetString and to use al_get_new_display_option instead of the _al_get_suggested_display_option, which is buggy and wrong. Further work needs to be done to fix that problem.

patch.txt

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 25, 2018

This affects X as well, I will make a further patch once I can test this on Ubuntu.

@EdgarReynaldo

This comment has been minimized.

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Oct 25, 2018

@EdgarReynaldo Is there any reason you opened that pull against your own fork rather than the main repo?

@dos1

This comment has been minimized.

Copy link
Contributor

dos1 commented Oct 25, 2018

Also, doesn't that patch break OpenGL < 3.0 contexts? Remember that due to its reliance on compatibility profiles, Allegro is still limited to OpenGL 2.1 on macOS.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 25, 2018

Lol I have no idea, it was just the cleanest way for me to keep master synced with allegro.

glGetStringi is valid on < 3 afaik, the function is a misnomer ( print_extensions_3_0 ) .

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 25, 2018

Is there any reason you opened that pull against your own fork rather than the main repo?

It's to prevent extra commits

@dos1

This comment has been minimized.

Copy link
Contributor

dos1 commented Oct 25, 2018

glGetStringi was introduced in OpenGL 3.0.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 25, 2018

This says core since 1.0 : https://www.khronos.org/opengl/wiki/GLAPI/glGetString, but of course there is no separate entry for glGetStringi so if they have some history I don't know. And you can always pre test glGetString(GL_EXTENSIONS) for null, to see if its available. I can modify the patch.

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Oct 25, 2018

This is why I prefer the manpage-style docs over the wiki:
https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glGetString.xhtml

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 25, 2018

Well, that actually lists the versions, which is far preferable. So yeah, I'll change things up and submit a new pull request.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 25, 2018

Adjusted the pull request. It should handle contexts < 3 now.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 26, 2018

Now it doesn't handle OPENGLES, let me fix it first.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Oct 28, 2018

Okay, now the pull request should handle OpenGLES and versions of OpenGL less than 3 properly, using glGetStringi when OpenGL version is greater than or equal to three.

I also added a patch to fill in the fields for ALLEGRO_OPENGL_MAJOR_VERSION and ALLEGRO_OPENGL_MINOR_VERSION so al_get_display_option works properly. They were never filled in before so they always returned 0.

@EdgarReynaldo

This comment has been minimized.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Dec 8, 2018

Can someone review my pull request please? It's been sitting here for a month.

@SiegeLord

This comment has been minimized.

Copy link
Member

SiegeLord commented Dec 8, 2018

Err, you made the pull request on your own repository... it needs to be made on this repository for us to notice it :).

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Dec 9, 2018

Nevermind, I should have it fixed now, sorry about that. New to this thing a ma jiggy.

@EdgarReynaldo

This comment has been minimized.

Copy link
Contributor

EdgarReynaldo commented Dec 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.