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

Allow re-creation of the shared context as a core context #1443

Merged
merged 1 commit into from Oct 23, 2018

Conversation

binary1248
Copy link
Member

Allow re-creation of the shared context as a core context if the user indicates they want a core profile context. Sharing of compatibility and core profile contexts is not possible on macOS which is why we need to have a way to re-create the shared context as a core context if required in this case.

The following code breaks on macOS without this change:

#include <SFML/Window.hpp>

int main()
{
    sf::Window window(sf::VideoMode(100, 100), "Test", sf::Style::Default, sf::ContextSettings(0, 0, 0, 3, 2, sf::ContextSettings::Core));

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.display();
    }
}

Copy link
Member

@mantognini mantognini left a comment

Choose a reason for hiding this comment

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

I've tested the provide program and the patch indeed fixes it. I haven't tested more complex programs though.

{
const char* extension = extensionString;

while(*extensionString && (*extensionString != ' '))
Copy link
Member

Choose a reason for hiding this comment

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

missing space: while_(

@@ -446,6 +471,25 @@ GlContext* GlContext::create(const ContextSettings& settings, unsigned int width

Lock lock(mutex);

// If resourceCount is 1 we know that we are inside sf::Context or sf::Window
Copy link
Member

Choose a reason for hiding this comment

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

DRY code: re-factor this code and the identical one from above into a function. Maybe a template function to accommodate for the width/height vs owner/bpp difference? If the logic is the same (except how context is constructed), I think it makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially tried to do this, but GlContext::initialize() is private.

// The shared context is the context used to initialize the extensions
if (shared && shared->m_deviceContext)
ensureExtensionsInit(shared->m_deviceContext);

// Create the rendering surface (window or pbuffer if supported)
createSurface(shared, width, height, VideoMode::getDesktopMode().bitsPerPixel);

// Create the context
if (m_deviceContext)
Copy link
Member

Choose a reason for hiding this comment

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

If possible, I would also DRY the ctors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Delegating constructors are coming in C++11. 😉

Copy link
Member

Choose a reason for hiding this comment

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

Good! Maybe add a \todo so we actually clean up the code at some point?

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation May 21, 2018
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone May 21, 2018
@binary1248 binary1248 force-pushed the feature/core_shared_context branch from 48b1eef to eb2316d Compare May 21, 2018 21:51
@binary1248
Copy link
Member Author

There... should be a bit better now. The amount of code duplication can probably be significantly reduced in C++11. I wouldn't invest too much time in trying to minimize the lines of code right now.

@binary1248 binary1248 force-pushed the feature/core_shared_context branch from eb2316d to 769d58e Compare June 3, 2018 16:00
@eXpl0it3r eXpl0it3r moved this from Discussion to Ready in SFML 2.6.0 Aug 13, 2018
… indicates they want a core profile context. Sharing of compatibility and core profile contexts is not possible on macOS which is why we need to have a way to re-create the shared context as a core context if required in this case.
@eXpl0it3r eXpl0it3r merged commit ac98be7 into master Oct 23, 2018
SFML 2.6.0 automation moved this from Ready to Done Oct 23, 2018
@binary1248 binary1248 deleted the feature/core_shared_context branch January 5, 2019 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants