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

Android: Copy the selected EGL context's settings to SFML #1039

Merged
merged 1 commit into from Feb 22, 2016

Conversation

Projects
None yet
4 participants
@MarioLiebisch
Member

MarioLiebisch commented Jan 7, 2016

This fixes the bug described in this forum thread.

In short, SFML picks a matching OpenGL ES context config, but won't copy that information back to it's internal sf::ContextSettings object for querying.

Open for comments and review/testing.

@MarioLiebisch MarioLiebisch self-assigned this Jan 7, 2016

@MarioLiebisch MarioLiebisch added this to the 2.4 milestone Jan 7, 2016

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 7, 2016

Member

Looks good.

However this file would really need some cleaning... I'll post comments here, I'm not sure it deserves a separate PR.

Member

LaurentGomila commented Jan 7, 2016

Looks good.

However this file would really need some cleaning... I'll post comments here, I'm not sure it deserves a separate PR.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 7, 2016

Member
     // On Android, its native activity handles this for us
     sf::priv::ActivityStates* states = sf::priv::getActivity(NULL);
     sf::Lock lock(states->mutex);

     return states->display;

-> Misses one level of indentation

 EglContext::EglContext(EglContext* shared) :
 m_display (EGL_NO_DISPLAY),
 m_context (EGL_NO_CONTEXT),
 m_surface (EGL_NO_SURFACE),
 m_config  (NULL)

-> Useless empty space between member names and (

    EGLint attrib_list[] = {

-> { should be under

#if !defined(SFML_SYSTEM_ANDROID)
    // Create EGL surface (except on Android because the window is created
    // asynchronously, its activity manager will call it for us)
    createSurface((EGLNativeWindowType)owner->getSystemHandle());
#endif

-> Needs some empty space :)

    if (currentContext == m_context)
    {
        eglCheck(eglMakeCurrent(m_display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT));
    }

    // Destroy context
    if (m_context != EGL_NO_CONTEXT)
    {
        eglCheck(eglDestroyContext(m_display, m_context));
    }

    // Destroy surface
    if (m_surface != EGL_NO_SURFACE)
    {
        eglCheck(eglDestroySurface(m_display, m_surface));
    }

-> All these { } should be removed

    EGLContext toShared;

    if (shared)
        toShared = shared->m_context;
    else
        toShared = EGL_NO_CONTEXT;

    // Create EGL context
    m_context = eglCheck(eglCreateContext(m_display, m_config, toShared, contextVersion));

->
m_context = eglCheck(eglCreateContext(m_display, m_config, shared ? shared->m_context : EGL_NO_CONTEXT, contextVersion));

Member

LaurentGomila commented Jan 7, 2016

     // On Android, its native activity handles this for us
     sf::priv::ActivityStates* states = sf::priv::getActivity(NULL);
     sf::Lock lock(states->mutex);

     return states->display;

-> Misses one level of indentation

 EglContext::EglContext(EglContext* shared) :
 m_display (EGL_NO_DISPLAY),
 m_context (EGL_NO_CONTEXT),
 m_surface (EGL_NO_SURFACE),
 m_config  (NULL)

-> Useless empty space between member names and (

    EGLint attrib_list[] = {

-> { should be under

#if !defined(SFML_SYSTEM_ANDROID)
    // Create EGL surface (except on Android because the window is created
    // asynchronously, its activity manager will call it for us)
    createSurface((EGLNativeWindowType)owner->getSystemHandle());
#endif

-> Needs some empty space :)

    if (currentContext == m_context)
    {
        eglCheck(eglMakeCurrent(m_display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT));
    }

    // Destroy context
    if (m_context != EGL_NO_CONTEXT)
    {
        eglCheck(eglDestroyContext(m_display, m_context));
    }

    // Destroy surface
    if (m_surface != EGL_NO_SURFACE)
    {
        eglCheck(eglDestroySurface(m_display, m_surface));
    }

-> All these { } should be removed

    EGLContext toShared;

    if (shared)
        toShared = shared->m_context;
    else
        toShared = EGL_NO_CONTEXT;

    // Create EGL context
    m_context = eglCheck(eglCreateContext(m_display, m_config, toShared, contextVersion));

->
m_context = eglCheck(eglCreateContext(m_display, m_config, shared ? shared->m_context : EGL_NO_CONTEXT, contextVersion));

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jan 7, 2016

Member

I'd say let's skip the cleanup for now. As mentioned in the added comment, I don't think parts of the current implementation are ideal (but it's unrelated to this bug; e.g. I think matched configs are not sorted), so there might be even more changes than just that formatting.

Member

MarioLiebisch commented Jan 7, 2016

I'd say let's skip the cleanup for now. As mentioned in the added comment, I don't think parts of the current implementation are ideal (but it's unrelated to this bug; e.g. I think matched configs are not sorted), so there might be even more changes than just that formatting.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 15, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Jan 15, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 26, 2016

Member

@MarioLiebisch himself raised a concern. ;)

Member

TankOs commented Jan 26, 2016

@MarioLiebisch himself raised a concern. ;)

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Jan 26, 2016

Member

@TankOs Nah, it's fine. While still not perfect as a whole, this PR fixes a clearly broken feature that is only related to the potentially weird context selection. We've got two bugs here (one unconfirmed), so nothing wrong in fixing this one first.

Member

MarioLiebisch commented Jan 26, 2016

@TankOs Nah, it's fine. While still not perfect as a whole, this PR fixes a clearly broken feature that is only related to the potentially weird context selection. We've got two bugs here (one unconfirmed), so nothing wrong in fixing this one first.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 26, 2016

Member

Ah alright, got it wrong then.

Member

TankOs commented Jan 26, 2016

Ah alright, got it wrong then.

@eXpl0it3r eXpl0it3r merged commit 499eb09 into master Feb 22, 2016

16 checks passed

debian-gcc-64 Build #83 done.
Details
freebsd-gcc-64 Build #83 done.
Details
osx-clang-universal Build #83 done.
Details
static-analysis Build #83 done.
Details
windows-gcc-471-tdm-32 Build #85 done.
Details
windows-gcc-471-tdm-64 Build #85 done.
Details
windows-gcc-481-tdm-32 Build #85 done.
Details
windows-gcc-481-tdm-64 Build #85 done.
Details
windows-gcc-520-mingw-32 Build #83 done.
Details
windows-gcc-520-mingw-64 Build #85 done.
Details
windows-vc11-32 Build #84 done.
Details
windows-vc11-64 Build #85 done.
Details
windows-vc12-32 Build #84 done.
Details
windows-vc12-64 Build #83 done.
Details
windows-vc14-32 Build #83 done.
Details
windows-vc14-64 Build #85 done.
Details

@eXpl0it3r eXpl0it3r deleted the bugfix/android_contextinfo branch Feb 22, 2016

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