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

Added missing setActive virtual method to sf::RenderTarget, added set… #1157

Merged
merged 1 commit into from Oct 13, 2016

Conversation

binary1248
Copy link
Member

Added missing setActive() virtual method to sf::RenderTarget and added setActive() calls to the OpenGL example to demonstrate proper explicit context management.

Discussions here and here.

@binary1248 binary1248 self-assigned this Oct 6, 2016
@eXpl0it3r eXpl0it3r added this to the 2.4.1 milestone Oct 6, 2016
Copy link
Member

@LaurentGomila LaurentGomila left a comment

Choose a reason for hiding this comment

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

Good for me. I'd just call setActive(true) instead of setActive() in the OpenGL example, for symmetry reasons.

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.

Nitpicking on the doc -- I haven't played with context for a while so those are the questions I've been asking myself while reading this PR. Sorry if those are questions have too obvious answers. :-)

/// Only one context can be current in a thread, so if you
/// want to draw OpenGL geometry to another render target
/// don't forget to activate it again.
///
Copy link
Member

Choose a reason for hiding this comment

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

For RenderWindow::setActive it is specified that the previously active window (if any) will get deactivated. Here no such statement being made leaves room for interpretation. The expected behaviour is probably something obvious for someone with good knowledge of OpenGL, but I'd still recommend adding a similar statement here or, if applicable, warn the user that (s)he should make sure the previous RT was deactivated (not sure if that last statement would make sense though).

Copy link
Member

Choose a reason for hiding this comment

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

Regarding multithreading, the documentation for RenderWindow::setActive seems more strong than this one. Should the user be expected to obey the following rule here as well?

if you want to make it active on another thread you have to deactivate it on the previous thread first if it was active.

…Active calls to OpenGL example to demonstrate proper explicit context management.
@binary1248
Copy link
Member Author

Fixed the raised concerns.

@eXpl0it3r
Copy link
Member

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

@eXpl0it3r eXpl0it3r merged commit 428c481 into 2.4.x Oct 13, 2016
@eXpl0it3r eXpl0it3r deleted the bugfix/set_active branch October 13, 2016 15:02
@eXpl0it3r eXpl0it3r modified the milestones: 2.5, 2.4.1 Nov 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Development

Successfully merging this pull request may close these issues.

None yet

4 participants