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

Removed internal OpenGL contexts #1002

Merged
merged 1 commit into from Oct 6, 2016

Conversation

Projects
None yet
@binary1248
Member

binary1248 commented Nov 14, 2015

Discussion prior to this pull request: http://en.sfml-dev.org/forums/index.php?topic=19106.0

Overview of changes:

  • Removed SFML's requirement of constantly having to keep a context active on each thread in order to manage GL objects
  • Implement actual context deactivation as a replacement for "activate something else to implicitly deactivate" (this will require changes in user code if they assumed this behaviour)
  • Requires more explicit context management in user code (i.e. activate before GL and deactivate after GL) as opposed to "activate once and forget". This leads to more predictable context behaviour i.e. you can be sure the context you activate will still be active after a call into a GLResource object
  • Reduced the number of temporary contexts that get created when an application starts up in some cases
  • Made context activation/deactivation behaviour during construction/destruction uniform across all platform context implementations
@eigenbom

This comment has been minimized.

Show comment
Hide comment
@eigenbom

eigenbom Jan 7, 2016

This is a good step towards a single-context SFML.

eigenbom commented Jan 7, 2016

This is a good step towards a single-context SFML.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 15, 2016

Member

Fixes #989.

Member

binary1248 commented Jan 15, 2016

Fixes #989.

@therocode

This comment has been minimized.

Show comment
Hide comment
@therocode

therocode Feb 21, 2016

Just going to mention that for me this feature branch fixes the issue of the endless context-ensuring loop that I saw when using a thread pool to load many sf::Textures prior to window creation. It introduces no other apparent issues for me, although I am only using SFML for texture loading in my application.

Not sure if relevant, but my system is 64 bit Arch Linux.

What is needed for this feature to be merged?

therocode commented Feb 21, 2016

Just going to mention that for me this feature branch fixes the issue of the endless context-ensuring loop that I saw when using a thread pool to load many sf::Textures prior to window creation. It introduces no other apparent issues for me, although I am only using SFML for texture loading in my application.

Not sure if relevant, but my system is 64 bit Arch Linux.

What is needed for this feature to be merged?

@therocode

This comment has been minimized.

Show comment
Hide comment
@therocode

therocode Feb 21, 2016

Actually, when testing this more, it seems like I am getting a deadlock. This is the backtrace: http://hastebin.com/bogesirare.vala

This is again using a threadpool where every worker in the pool executes the following loading code on the given textures, one by one in parallel: http://hastebin.com/ehoxigosin.coffee

My application is this: http://hastebin.com/zulasasova.cpp

The ResourceProvider is pretty much just an encapsulated threadpool which loads the textures through the function shown in the other paste above. When tweaking things, I found that the deadlock is avoided if any of these two conditions are true:

  1. The threadpool only has 1 worker
  2. .loadFromFile is called on a texture in the main thread before dispatching the async load

#1 is not surprising since only a single extra thread can't deadlock itself I guess, but #2 is interesting. It seems like the deadlock doesn't happen if the main thread has first loaded an arbitrary texture.

Further source and info can be provided if needed.

therocode commented Feb 21, 2016

Actually, when testing this more, it seems like I am getting a deadlock. This is the backtrace: http://hastebin.com/bogesirare.vala

This is again using a threadpool where every worker in the pool executes the following loading code on the given textures, one by one in parallel: http://hastebin.com/ehoxigosin.coffee

My application is this: http://hastebin.com/zulasasova.cpp

The ResourceProvider is pretty much just an encapsulated threadpool which loads the textures through the function shown in the other paste above. When tweaking things, I found that the deadlock is avoided if any of these two conditions are true:

  1. The threadpool only has 1 worker
  2. .loadFromFile is called on a texture in the main thread before dispatching the async load

#1 is not surprising since only a single extra thread can't deadlock itself I guess, but #2 is interesting. It seems like the deadlock doesn't happen if the main thread has first loaded an arbitrary texture.

Further source and info can be provided if needed.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 21, 2016

Member

If you suspect a deadlock, it might be caused in your code. I can't think of any cycles that might be present in SFML code since the dependencies basically only go in one direction.

You'll need to provide the callstacks of your other threads as well.

Member

binary1248 commented Feb 21, 2016

If you suspect a deadlock, it might be caused in your code. I can't think of any cycles that might be present in SFML code since the dependencies basically only go in one direction.

You'll need to provide the callstacks of your other threads as well.

@therocode

This comment has been minimized.

Show comment
Hide comment
@therocode

therocode Feb 21, 2016

Ok, I did a test with 2 workers in the pool, that makes 3 threads with the main thread included.

Gdb thread listing: http://hastebin.com/fipimidefe.coffee

The main thread is not locked, it spins in the loop printing that 0% of the textures are loaded over and over.

Worker threads:
Thread 2: http://hastebin.com/mihoqedoko.vala
Thread 3: http://hastebin.com/luyoxivosu.vala

No other threads exist.

It seems to me like if the main thread is not locked, and both worker threads are locked inside the sfml texture loadFromFile method that it somehow must be inside SFML's code? Is it possible for my external code to cause this somehow? (assuming I'm not corrupting the stack or anything such)

therocode commented Feb 21, 2016

Ok, I did a test with 2 workers in the pool, that makes 3 threads with the main thread included.

Gdb thread listing: http://hastebin.com/fipimidefe.coffee

The main thread is not locked, it spins in the loop printing that 0% of the textures are loaded over and over.

Worker threads:
Thread 2: http://hastebin.com/mihoqedoko.vala
Thread 3: http://hastebin.com/luyoxivosu.vala

No other threads exist.

It seems to me like if the main thread is not locked, and both worker threads are locked inside the sfml texture loadFromFile method that it somehow must be inside SFML's code? Is it possible for my external code to cause this somehow? (assuming I'm not corrupting the stack or anything such)

@therocode

This comment has been minimized.

Show comment
Hide comment
@therocode

therocode Feb 21, 2016

Here is additional information from running with helgrind, giving what looks like threading errors originating from the SFML functions.

http://hastebin.com/qozayicaxa.coffee

I have never used this error checker before so I don't know if it is generous with false positives but it looks like it is suspecting a data race in stbi__load and also some lock ordering issues. Not sure if related to the deadlock. In the end when I exited with ctrl-c you can see the deadlock of thread 2 and 3

therocode commented Feb 21, 2016

Here is additional information from running with helgrind, giving what looks like threading errors originating from the SFML functions.

http://hastebin.com/qozayicaxa.coffee

I have never used this error checker before so I don't know if it is generous with false positives but it looks like it is suspecting a data race in stbi__load and also some lock ordering issues. Not sure if related to the deadlock. In the end when I exited with ctrl-c you can see the deadlock of thread 2 and 3

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 21, 2016

Member

Give the version I just pushed a try.

Member

binary1248 commented Feb 21, 2016

Give the version I just pushed a try.

@therocode

This comment has been minimized.

Show comment
Hide comment
@therocode

therocode Feb 21, 2016

Wow, that does work fine indeed. Awesome.

therocode commented Feb 21, 2016

Wow, that does work fine indeed. Awesome.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 6, 2016

Member

I'm currently experimenting with SFML alongside Irrlicht -- i.e. two OpenGL renderers -- and even creating one independent window in each library has led to flickering so far. With this branch, the flickering has disappeared! I even managed to draw a SFML sprite onto an Irrlicht window (some GL states are still messed up, which I need to take care of manually). Great work! 👍

Member

Bromeon commented Mar 6, 2016

I'm currently experimenting with SFML alongside Irrlicht -- i.e. two OpenGL renderers -- and even creating one independent window in each library has led to flickering so far. With this branch, the flickering has disappeared! I even managed to draw a SFML sprite onto an Irrlicht window (some GL states are still messed up, which I need to take care of manually). Great work! 👍

@TheoVerhelst

This comment has been minimized.

Show comment
Hide comment
@TheoVerhelst

TheoVerhelst May 4, 2016

The following code, that was resulting in a segfault similar as the one described in #989 when using master (2c7b58f), works perfectly fine on this branch.

#include <thread>
#include <SFML/Graphics.hpp>

void threadFunction()
{
    sf::Texture tex;
}

int main()
{
    sf::RenderWindow window(sf::VideoMode(200, 200), "Window");
    std::thread thread(&threadFunction);

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event));
        window.clear();
        window.display();
    }
}

TheoVerhelst commented May 4, 2016

The following code, that was resulting in a segfault similar as the one described in #989 when using master (2c7b58f), works perfectly fine on this branch.

#include <thread>
#include <SFML/Graphics.hpp>

void threadFunction()
{
    sf::Texture tex;
}

int main()
{
    sf::RenderWindow window(sf::VideoMode(200, 200), "Window");
    std::thread thread(&threadFunction);

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event));
        window.clear();
        window.display();
    }
}
@ekunuke

This comment has been minimized.

Show comment
Hide comment
@ekunuke

ekunuke Jun 15, 2016

I've been doing some testing with this feature branch, but I've run into an issue.

It only seems to occur on some Windows 7 machines (which I don't have access to). It doesn't seem to occur on all Windows 7 machines, as it runs fine in my Win7 VM.

When first creating the window, this is printed:

Failed to deactivate shared context before sharing: The handle is invalid.

Warning: The created OpenGL context does not fully meet the settings that were requested
Requested: version = 1.1 ; depth bits = 0 ; stencil bits = 0 ; AA level = 0 ; core = false ; debug = false
Created: version = 0.0 ; depth bits = 0 ; stencil bits = 0 ; AA level = 0 ; core = false ; debug = false
Failed to activate the window's context
Failed to activate the window's context
Failed to activate the window's context
Failed to activate the window's context

I don't have any more details at the moment, but maybe one of you has some idea of why this may be happening.

ekunuke commented Jun 15, 2016

I've been doing some testing with this feature branch, but I've run into an issue.

It only seems to occur on some Windows 7 machines (which I don't have access to). It doesn't seem to occur on all Windows 7 machines, as it runs fine in my Win7 VM.

When first creating the window, this is printed:

Failed to deactivate shared context before sharing: The handle is invalid.

Warning: The created OpenGL context does not fully meet the settings that were requested
Requested: version = 1.1 ; depth bits = 0 ; stencil bits = 0 ; AA level = 0 ; core = false ; debug = false
Created: version = 0.0 ; depth bits = 0 ; stencil bits = 0 ; AA level = 0 ; core = false ; debug = false
Failed to activate the window's context
Failed to activate the window's context
Failed to activate the window's context
Failed to activate the window's context

I don't have any more details at the moment, but maybe one of you has some idea of why this may be happening.

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Jun 15, 2016

Contributor

@ekunuke That looks like these machines do not have proper OpenGL-drivers installed. Does that happen with any SFML version or just with this feature-branch?

Contributor

BlueCobold commented Jun 15, 2016

@ekunuke That looks like these machines do not have proper OpenGL-drivers installed. Does that happen with any SFML version or just with this feature-branch?

@ekunuke

This comment has been minimized.

Show comment
Hide comment
@ekunuke

ekunuke Jun 15, 2016

It only happens with this feature branch. Our stable branch which is SFML version 2.3.2, does not have this issue.

ekunuke commented Jun 15, 2016

It only happens with this feature branch. Our stable branch which is SFML version 2.3.2, does not have this issue.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jun 15, 2016

Member

If the code you used to test this branch is simple, can you post it here?

Member

binary1248 commented Jun 15, 2016

If the code you used to test this branch is simple, can you post it here?

@ekunuke

This comment has been minimized.

Show comment
Hide comment
@ekunuke

ekunuke Jun 15, 2016

The original code isn't simple, but I'll try making the minimal code that reproduces the issue. It will take some time though since it doesn't occur on my machine.

ekunuke commented Jun 15, 2016

The original code isn't simple, but I'll try making the minimal code that reproduces the issue. It will take some time though since it doesn't occur on my machine.

@Estivoo

This comment has been minimized.

Show comment
Hide comment
@Estivoo

Estivoo Jun 25, 2016

@binary1248 same issue on android 5.1.1 OnePlus X. I will try to solve it tomorrow.

Estivoo commented Jun 25, 2016

@binary1248 same issue on android 5.1.1 OnePlus X. I will try to solve it tomorrow.

@eXpl0it3r eXpl0it3r added this to the 2.5 milestone Aug 13, 2016

@binary1248 binary1248 changed the base branch from master to 2.4.x Sep 10, 2016

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 10, 2016

Member

Bump.

Member

binary1248 commented Sep 10, 2016

Bump.

@eXpl0it3r eXpl0it3r modified the milestones: 2.4.1, 2.5 Sep 23, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 1, 2016

Member

@ekunuke @Estivoo any updates?

Member

eXpl0it3r commented Oct 1, 2016

@ekunuke @Estivoo any updates?

@eXpl0it3r

Seems to fix the stack overflow when running this test code:

#include <SFML/Graphics.hpp>
#include <thread>

int main()
{
    sf::Window w;
    auto thread = std::thread{ [] 
    { 
        sf::Texture t;  // Stack overflow occurs here
    }};
    thread.join();
}

@eXpl0it3r eXpl0it3r added s:accepted and removed s:undecided labels Oct 4, 2016

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 4, 2016

Member

I quickly tested the snippet posted by @eXpl0it3r on OS X and can confirm that: the bug is present on branch 2.4.1 and the crash is gone using the feature/no_internal_context branch. 👍

Member

mantognini commented Oct 4, 2016

I quickly tested the snippet posted by @eXpl0it3r on OS X and can confirm that: the bug is present on branch 2.4.1 and the crash is gone using the feature/no_internal_context branch. 👍

Removed internal OpenGL contexts, reduced the number of temporary con…
…texts that get created during runtime.
@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 4, 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 Oct 4, 2016

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

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Oct 4, 2016

Contributor

The issue with failing glLoadGen on Android still remains though. I know I was asked to provide a sample and I didn't yet, but I still wanted to mention it. This will probably cause issues for anybody wanting to use direct GL calls via glLoadGen on Android. This issue appears in this branch only and worked properly prior to these changes.

Contributor

BlueCobold commented Oct 4, 2016

The issue with failing glLoadGen on Android still remains though. I know I was asked to provide a sample and I didn't yet, but I still wanted to mention it. This will probably cause issues for anybody wanting to use direct GL calls via glLoadGen on Android. This issue appears in this branch only and worked properly prior to these changes.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 4, 2016

Member

The issue with failing glLoadGen on Android still remains though.

This is assuming it is even an issue.

I know I was asked to provide a sample and I didn't yet, but I still wanted to mention it.

"Doubts" have been mentioned time and again, but until now there has not been a single concrete example on which to base any improvement efforts. There is no point blocking progress for the general community if one is not able to reliably differentiate between user error or any true defect.

This will probably cause issues for anybody wanting to use direct GL calls via glLoadGen on Android. This issue appears in this branch only and worked properly prior to these changes.

I've already mentioned multiple times that this change breaks undocumented assumptions that were more harmful than they were helpful in the past. If code is based around these assumptions then it is good that it breaks, because it shows that this PR actually has an effect on program execution, hopefully a generally positive effect. There is no behaviour changing patch that is guaranteed not to have an effect on any already written code. The important thing is being able to differentiate between which state is the better, the one before or the one after. Without providing two samples to compare against each other I wouldn't directly deem a change as one that shouldn't be made for the sake of progress.

Member

binary1248 commented Oct 4, 2016

The issue with failing glLoadGen on Android still remains though.

This is assuming it is even an issue.

I know I was asked to provide a sample and I didn't yet, but I still wanted to mention it.

"Doubts" have been mentioned time and again, but until now there has not been a single concrete example on which to base any improvement efforts. There is no point blocking progress for the general community if one is not able to reliably differentiate between user error or any true defect.

This will probably cause issues for anybody wanting to use direct GL calls via glLoadGen on Android. This issue appears in this branch only and worked properly prior to these changes.

I've already mentioned multiple times that this change breaks undocumented assumptions that were more harmful than they were helpful in the past. If code is based around these assumptions then it is good that it breaks, because it shows that this PR actually has an effect on program execution, hopefully a generally positive effect. There is no behaviour changing patch that is guaranteed not to have an effect on any already written code. The important thing is being able to differentiate between which state is the better, the one before or the one after. Without providing two samples to compare against each other I wouldn't directly deem a change as one that shouldn't be made for the sake of progress.

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Oct 4, 2016

Contributor

it shows that this PR actually has an effect on program execution

Yea. Although I don't think anybody denied the great improvement this PR will bring. It would be quite bad though if it will break usage of explicit OpenGL calls in general. I doubt SFML would like this to happen. Did you try running the SFML-provided OpenGL-sample application to see if it could be general issue? You know, I'm short on time and I obviously can't guarantee the issue comes from my code or from this PR. But you also can't deny that the possibility exists. So when the typical recommendation for devs is to use glLoadGen in order to get access to OpenGL-calls, then it should be checked whether this is still possible or not.
You can merge if you don't care about this or think it's irrelevant. I'm still raising my concerns since it was asked for.

The important thing is being able to differentiate between which state is the better, the one before or the one after.

For me, both are a deal-breaker. Without it, my game crashes on Android. With it, I have other big issues on Android (due to not being able to call various GL functions via glLoadGen).

Contributor

BlueCobold commented Oct 4, 2016

it shows that this PR actually has an effect on program execution

Yea. Although I don't think anybody denied the great improvement this PR will bring. It would be quite bad though if it will break usage of explicit OpenGL calls in general. I doubt SFML would like this to happen. Did you try running the SFML-provided OpenGL-sample application to see if it could be general issue? You know, I'm short on time and I obviously can't guarantee the issue comes from my code or from this PR. But you also can't deny that the possibility exists. So when the typical recommendation for devs is to use glLoadGen in order to get access to OpenGL-calls, then it should be checked whether this is still possible or not.
You can merge if you don't care about this or think it's irrelevant. I'm still raising my concerns since it was asked for.

The important thing is being able to differentiate between which state is the better, the one before or the one after.

For me, both are a deal-breaker. Without it, my game crashes on Android. With it, I have other big issues on Android (due to not being able to call various GL functions via glLoadGen).

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 4, 2016

Member

I use SFML extensively with modern (core) OpenGL and thus I also indirectly test everything against the current development version of SFML on my machine as well as in the VMs and secondary operating systems I develop SFML on. I am also currently using glLoadGen to load the entry points just as you mentioned in your cases. This is why concrete examples are of utmost importance here. If it was that easy for me to reproduce what can easily be said about "some things might break in a way that is non-trivial to reproduce in a short example" then I would have already caught the probably obvious fault over a year ago when I wrote the initial draft of this PR.

This PR improves OpenGL performance for me, in my OpenGL-focused projects without any negative side effects. What also has to be said is that I am aware of the old as well as the new undocumented side effects and thus I don't write code assuming any of them. It is harder for me to follow this guideline because of the inherent information I have about the implementation due to actively maintaining it. All I ask is to at least try to review this code that might raise some questions and compare it to the clearly documented behaviour. If the documented behaviour does not match the actual behaviour and because of that code breaks, I'm all open to constructive feedback. We just cannot afford to hinder progress on this PR any longer based on what seems little more than "gut feeling" at the moment...

Member

binary1248 commented Oct 4, 2016

I use SFML extensively with modern (core) OpenGL and thus I also indirectly test everything against the current development version of SFML on my machine as well as in the VMs and secondary operating systems I develop SFML on. I am also currently using glLoadGen to load the entry points just as you mentioned in your cases. This is why concrete examples are of utmost importance here. If it was that easy for me to reproduce what can easily be said about "some things might break in a way that is non-trivial to reproduce in a short example" then I would have already caught the probably obvious fault over a year ago when I wrote the initial draft of this PR.

This PR improves OpenGL performance for me, in my OpenGL-focused projects without any negative side effects. What also has to be said is that I am aware of the old as well as the new undocumented side effects and thus I don't write code assuming any of them. It is harder for me to follow this guideline because of the inherent information I have about the implementation due to actively maintaining it. All I ask is to at least try to review this code that might raise some questions and compare it to the clearly documented behaviour. If the documented behaviour does not match the actual behaviour and because of that code breaks, I'm all open to constructive feedback. We just cannot afford to hinder progress on this PR any longer based on what seems little more than "gut feeling" at the moment...

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Oct 4, 2016

Contributor

I also indirectly test everything against the current development version of SFML on my machine as well as in the VMs and secondary operating systems

That doesn't include Android though? About documented behaviour... I'm still wondering where such a document exactly is - i.e. "official documentation about correctly using external gl-function-loaders".
It has little to do with "gut" feelings. Fact is, I can't load any gl-function-entry-points at the moment on Android with this PR. No matter where I place my code - before window-creation, after creation, after first (successfull) rendering, glLoadGen fails to get any gl-entry-point dynamically.

Contributor

BlueCobold commented Oct 4, 2016

I also indirectly test everything against the current development version of SFML on my machine as well as in the VMs and secondary operating systems

That doesn't include Android though? About documented behaviour... I'm still wondering where such a document exactly is - i.e. "official documentation about correctly using external gl-function-loaders".
It has little to do with "gut" feelings. Fact is, I can't load any gl-function-entry-points at the moment on Android with this PR. No matter where I place my code - before window-creation, after creation, after first (successfull) rendering, glLoadGen fails to get any gl-entry-point dynamically.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 4, 2016

Member

Fact is, I can't load any gl-function-entry-points at the moment on Android with this PR. No matter where I place my code - before window-creation, after creation, after first (successfull) rendering, glLoadGen fails to get any gl-entry-point dynamically.

If it is a fact and you have tested various variations, why can't you provide the code you used to test it?

Member

eXpl0it3r commented Oct 4, 2016

Fact is, I can't load any gl-function-entry-points at the moment on Android with this PR. No matter where I place my code - before window-creation, after creation, after first (successfull) rendering, glLoadGen fails to get any gl-entry-point dynamically.

If it is a fact and you have tested various variations, why can't you provide the code you used to test it?

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Oct 4, 2016

Contributor

That code is even here on github, but it's not a minimal sample.

Contributor

BlueCobold commented Oct 4, 2016

That code is even here on github, but it's not a minimal sample.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 4, 2016

Member

Well if you could link to the code that deals with Android and custom OpenGL calls or glLoadGen loading, it would already be more information than we currently have. 😉

Member

eXpl0it3r commented Oct 4, 2016

Well if you could link to the code that deals with Android and custom OpenGL calls or glLoadGen loading, it would already be more information than we currently have. 😉

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Oct 4, 2016

Contributor

https://github.com/BlueCobold/forenprojekt/blob/master/Source/FirstParty/Src/App.cpp
Line 66-68. As I said, I can move that block around to different places (after window-creation or after rendering, etc) and it won't change anything. The function-pointers will stay all null. The called glLoadGen-functions fail to find/load any entry points.

Contributor

BlueCobold commented Oct 4, 2016

https://github.com/BlueCobold/forenprojekt/blob/master/Source/FirstParty/Src/App.cpp
Line 66-68. As I said, I can move that block around to different places (after window-creation or after rendering, etc) and it won't change anything. The function-pointers will stay all null. The called glLoadGen-functions fail to find/load any entry points.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 4, 2016

Member

As stated already in the initial description of this PR at the top of this page:

  • Removed SFML's requirement of constantly having to keep a context active on each thread in order to manage GL objects
  • Implement actual context deactivation as a replacement for "activate something else to implicitly deactivate" (this will require changes in user code if they assumed this behaviour)
  • Requires more explicit context management in user code (i.e. activate before GL and deactivate after GL) as opposed to "activate once and forget". This leads to more predictable context behaviour i.e. you can be sure the context you activate will still be active after a call into a GLResource object

A keen observer might also notice that this branch is indeed called feature/no_internal_context and wonder why such a name was chosen and what it might mean.

All these points apply to your code, and other code that makes the same assumptions, which is why I mentioned them right at the start. When you load your OpenGL entry points in your App constructor, the sf::RenderWindow would have been freshly created. SFML never in its history guaranteed that the window would be activated for OpenGL operations right after construction. The fact that it might be the case up to 2.4.0 was an implementation detail that was never documented because we knew that it could change at some point.

The canonical way of "protecting" your OpenGL operations from executing in an environment without an active context is obviously to make sure .setActive(true) is called on some render target before executing said operations and .setActive(false) is called after. This was the case before this PR and will still be the case after this PR. This is also the only documented way to ensure a context is active when you have to assume one is. Doing a GitHub search, it seems like sf::RenderWindow::setActive(true) is called in 2 places throughout your code, both of which are in DrawParameter::activatePrimary(). Considering the amount of OpenGL you are calling yourself, this is far too little, which is also why things fall apart after this PR. The fact that the contexts aren't deactivated accordingly when you are done with them via sf::RenderWindow::setActive(false) will also make your code fall apart after this PR.

Like I said, it is these kinds of assumptions that should not be made about SFML behaviour. They are bound to change due to internal refactoring at some point. I admit, maybe the documentation wasn't absolutely and ultimately explicit about having to make sure a context is active via calling .setActive() before executing any OpenGL code, but the opposite (not requiring to do so) was and will also never be documented. As such, assuming that the latter is guaranteed to be the case leads to such situations.

Member

binary1248 commented Oct 4, 2016

As stated already in the initial description of this PR at the top of this page:

  • Removed SFML's requirement of constantly having to keep a context active on each thread in order to manage GL objects
  • Implement actual context deactivation as a replacement for "activate something else to implicitly deactivate" (this will require changes in user code if they assumed this behaviour)
  • Requires more explicit context management in user code (i.e. activate before GL and deactivate after GL) as opposed to "activate once and forget". This leads to more predictable context behaviour i.e. you can be sure the context you activate will still be active after a call into a GLResource object

A keen observer might also notice that this branch is indeed called feature/no_internal_context and wonder why such a name was chosen and what it might mean.

All these points apply to your code, and other code that makes the same assumptions, which is why I mentioned them right at the start. When you load your OpenGL entry points in your App constructor, the sf::RenderWindow would have been freshly created. SFML never in its history guaranteed that the window would be activated for OpenGL operations right after construction. The fact that it might be the case up to 2.4.0 was an implementation detail that was never documented because we knew that it could change at some point.

The canonical way of "protecting" your OpenGL operations from executing in an environment without an active context is obviously to make sure .setActive(true) is called on some render target before executing said operations and .setActive(false) is called after. This was the case before this PR and will still be the case after this PR. This is also the only documented way to ensure a context is active when you have to assume one is. Doing a GitHub search, it seems like sf::RenderWindow::setActive(true) is called in 2 places throughout your code, both of which are in DrawParameter::activatePrimary(). Considering the amount of OpenGL you are calling yourself, this is far too little, which is also why things fall apart after this PR. The fact that the contexts aren't deactivated accordingly when you are done with them via sf::RenderWindow::setActive(false) will also make your code fall apart after this PR.

Like I said, it is these kinds of assumptions that should not be made about SFML behaviour. They are bound to change due to internal refactoring at some point. I admit, maybe the documentation wasn't absolutely and ultimately explicit about having to make sure a context is active via calling .setActive() before executing any OpenGL code, but the opposite (not requiring to do so) was and will also never be documented. As such, assuming that the latter is guaranteed to be the case leads to such situations.

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Oct 5, 2016

Contributor

A keen observer might also notice

No need to get personal, really. I will give your hints a try though. It might be crystal clear for you to use SFML that way, but it never was and imo does not spring up from the documentation either that it needs to be used like that.
It sounds a little shocking though that I will need to permantly activate and deactivate the context. Even worse due to the fact that sf::RenderTarget doesn't even offer this operation and needing to use sf::RenderWindow/RenderTexture everywhere instead of sf::RenderTarget is quite bad. That's also why I'm having it in quite a dirty way inside my DrawParameter-class. What do you recommend in this case when using sf::RenderTarget transparently in the depth of the own render-pipeline where it shouldn't matter much if the given target is a texture or a window? Calling .setActive() is impossible this way unless I want to start making other dirty casts all the time.

Note: The SFML-OpenGL-example also doesn't even call .setActive() even once. I guess this needs fixing then to properly reflect the way SFML should be used.

Contributor

BlueCobold commented Oct 5, 2016

A keen observer might also notice

No need to get personal, really. I will give your hints a try though. It might be crystal clear for you to use SFML that way, but it never was and imo does not spring up from the documentation either that it needs to be used like that.
It sounds a little shocking though that I will need to permantly activate and deactivate the context. Even worse due to the fact that sf::RenderTarget doesn't even offer this operation and needing to use sf::RenderWindow/RenderTexture everywhere instead of sf::RenderTarget is quite bad. That's also why I'm having it in quite a dirty way inside my DrawParameter-class. What do you recommend in this case when using sf::RenderTarget transparently in the depth of the own render-pipeline where it shouldn't matter much if the given target is a texture or a window? Calling .setActive() is impossible this way unless I want to start making other dirty casts all the time.

Note: The SFML-OpenGL-example also doesn't even call .setActive() even once. I guess this needs fixing then to properly reflect the way SFML should be used.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 5, 2016

Member

No need to get personal, really.

This is a general figure of speech in the English language and isn't directed at anyone in particular.

it never was and imo does not spring up from the documentation either that it needs to be used like that.

Yes... like I said, the documentation can be improved to be more explicit about this behaviour and the true purpose of .setActive(), however the lack of such explicit documentation or unfortunate misunderstanding of the circumstances must not be allowed to become the bottleneck when it comes to making progress in general.

Even worse due to the fact that sf::RenderTarget doesn't even offer this operation and needing to use sf::RenderWindow/RenderTexture everywhere instead of sf::RenderTarget is quite bad.

Yes... this is a slight anomaly in the API that I have also noticed quite some time ago. It might be improved at some point. I've started a related discussion here.

What do you recommend in this case when using sf::RenderTarget transparently in the depth of the own render-pipeline where it shouldn't matter much if the given target is a texture or a window? Calling .setActive() is impossible this way unless I want to start making other dirty casts all the time.

Some users have already gone the way of sub-classing sf::RenderTarget for other purposes, however I don't see why it can't also be done in this case as a temporary solution until the API can be improved. I wouldn't even consider it "dirty".

The SFML-OpenGL-example also doesn't even call .setActive() even once. I guess this needs fixing then to properly reflect the way SFML should be used.

Yes... there was in fact a time before commit 2752bbc where .setActive() was explicitly called in the example. Although it was no longer functionally necessary after that commit, I admit, it might serve the purpose of the example better if it were to be left in.

Member

binary1248 commented Oct 5, 2016

No need to get personal, really.

This is a general figure of speech in the English language and isn't directed at anyone in particular.

it never was and imo does not spring up from the documentation either that it needs to be used like that.

Yes... like I said, the documentation can be improved to be more explicit about this behaviour and the true purpose of .setActive(), however the lack of such explicit documentation or unfortunate misunderstanding of the circumstances must not be allowed to become the bottleneck when it comes to making progress in general.

Even worse due to the fact that sf::RenderTarget doesn't even offer this operation and needing to use sf::RenderWindow/RenderTexture everywhere instead of sf::RenderTarget is quite bad.

Yes... this is a slight anomaly in the API that I have also noticed quite some time ago. It might be improved at some point. I've started a related discussion here.

What do you recommend in this case when using sf::RenderTarget transparently in the depth of the own render-pipeline where it shouldn't matter much if the given target is a texture or a window? Calling .setActive() is impossible this way unless I want to start making other dirty casts all the time.

Some users have already gone the way of sub-classing sf::RenderTarget for other purposes, however I don't see why it can't also be done in this case as a temporary solution until the API can be improved. I wouldn't even consider it "dirty".

The SFML-OpenGL-example also doesn't even call .setActive() even once. I guess this needs fixing then to properly reflect the way SFML should be used.

Yes... there was in fact a time before commit 2752bbc where .setActive() was explicitly called in the example. Although it was no longer functionally necessary after that commit, I admit, it might serve the purpose of the example better if it were to be left in.

@eXpl0it3r eXpl0it3r merged commit cca38d9 into 2.4.x Oct 6, 2016

15 checks passed

android-armeabi-v7a-api13 Build #9 done.
Details
debian-gcc-64 Build #283 done.
Details
freebsd-gcc-64 Build #246 done.
Details
osx-clang-el-capitan Build #129 done.
Details
static-analysis Build #253 done.
Details
windows-gcc-492-tdm-32 Build #129 done.
Details
windows-gcc-492-tdm-64 Build #129 done.
Details
windows-gcc-610-mingw-32 Build #64 done.
Details
windows-gcc-610-mingw-64 Build #64 done.
Details
windows-vc11-32 Build #245 done.
Details
windows-vc11-64 Build #245 done.
Details
windows-vc12-32 Build #244 done.
Details
windows-vc12-64 Build #243 done.
Details
windows-vc14-32 Build #244 done.
Details
windows-vc14-64 Build #247 done.
Details

@eXpl0it3r eXpl0it3r deleted the feature/no_internal_context branch Oct 6, 2016

@tango-oscar

This comment has been minimized.

Show comment
Hide comment
@tango-oscar

tango-oscar Oct 15, 2016

I can confirm the "Failed to deactivate shared context before sharing" bug that @ekunuke reported on Windows 7 machine with NVIDIA graphics card when building this basic SFML example.

After updating graphics card drivers, the bug was gone.

I fixed the bug without updating the drivers by changing function calls on lines 575 and 640 of WglContex.cpp from wglMakeCurrent(NULL, NULL) to wglMakeCurrent(m_deviceContext, NULL).

tango-oscar commented Oct 15, 2016

I can confirm the "Failed to deactivate shared context before sharing" bug that @ekunuke reported on Windows 7 machine with NVIDIA graphics card when building this basic SFML example.

After updating graphics card drivers, the bug was gone.

I fixed the bug without updating the drivers by changing function calls on lines 575 and 640 of WglContex.cpp from wglMakeCurrent(NULL, NULL) to wglMakeCurrent(m_deviceContext, NULL).

@BlueCobold

This comment has been minimized.

Show comment
Hide comment
@BlueCobold

BlueCobold Nov 9, 2016

Contributor

Same issue here with a win7 NVidia machine. Results in no valid context-creation which basically means rendering and any opengl-related sfml functionality is broken entirely.
http://en.sfml-dev.org/forums/index.php?topic=21102

Contributor

BlueCobold commented Nov 9, 2016

Same issue here with a win7 NVidia machine. Results in no valid context-creation which basically means rendering and any opengl-related sfml functionality is broken entirely.
http://en.sfml-dev.org/forums/index.php?topic=21102

@hsdk123

This comment has been minimized.

Show comment
Hide comment
@hsdk123

hsdk123 Nov 21, 2016

I'm not sure if this is already implemented, but would it be possible to just create a context externally and feed it to SFML? If so, then this would be a mere matter of, (on the user side), checking whether the context's been created, and if not, create a custom context and feet it into SFML. This might make things more flexible.

In which event, SFML wouldn't have to handle ex. graphics card differences within the library, while giving the user freedom should he/she choose to do so.

hsdk123 commented Nov 21, 2016

I'm not sure if this is already implemented, but would it be possible to just create a context externally and feed it to SFML? If so, then this would be a mere matter of, (on the user side), checking whether the context's been created, and if not, create a custom context and feet it into SFML. This might make things more flexible.

In which event, SFML wouldn't have to handle ex. graphics card differences within the library, while giving the user freedom should he/she choose to do so.

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