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

Fixed Texture::getMaximumSize() breaking context management #666

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Foaly
Contributor

Foaly commented Jul 18, 2014

Fixed a bug where calling Texture::getMaximumSize() before any GlResource is created would break context management.

@binary1248 binary1248 added the bug label Jul 19, 2014

@binary1248 binary1248 added this to the 2.x milestone Jul 19, 2014

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 19, 2014

Member

I would also include this patch just for good measure and to protect against future manifestations of this problem 😄.

Member

binary1248 commented Jul 19, 2014

I would also include this patch just for good measure and to protect against future manifestations of this problem 😄.

Maximilian Wagenbach
Added an assertion, to be properly notified if the context management…
… breaks, because no GlResource was created prior.
@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jul 19, 2014

Contributor

Alright! I included your commit. Sadly git am was complaining that there was no valid email adress, thats why I couldn't use sign-off. (I think it's because its only a .diff and not a .patch)
I hope that's alright. If you want the ownership of that commit, send me a patch and I will include that one.
If everything else is alright I would love to see this integrated, so this nasty bug is fixed and I can keep developing with SFML master 😄

Contributor

Foaly commented Jul 19, 2014

Alright! I included your commit. Sadly git am was complaining that there was no valid email adress, thats why I couldn't use sign-off. (I think it's because its only a .diff and not a .patch)
I hope that's alright. If you want the ownership of that commit, send me a patch and I will include that one.
If everything else is alright I would love to see this integrated, so this nasty bug is fixed and I can keep developing with SFML master 😄

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 19, 2014

Member

I really can't care who makes any changes to SFML... I just want bugs to be fixed and features to be implemented.

Member

binary1248 commented Jul 19, 2014

I really can't care who makes any changes to SFML... I just want bugs to be fixed and features to be implemented.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jul 19, 2014

Contributor

haha awesome! thats exactly the right attitude!
Ok I think we are good to go then. Do we wait for somebody else to have a look at it?

Contributor

Foaly commented Jul 19, 2014

haha awesome! thats exactly the right attitude!
Ok I think we are good to go then. Do we wait for somebody else to have a look at it?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jul 19, 2014

Member

What did you change in Text.cpp? Line endings?

Otherwise it looks good to me.

Member

LaurentGomila commented Jul 19, 2014

What did you change in Text.cpp? Line endings?

Otherwise it looks good to me.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 19, 2014

Member

Actually... in Text.cpp and GlContext.cpp I got rid of the unused #include <cassert>, but I guess git fixed the line endings at the same time.

Member

binary1248 commented Jul 19, 2014

Actually... in Text.cpp and GlContext.cpp I got rid of the unused #include <cassert>, but I guess git fixed the line endings at the same time.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jul 19, 2014

Contributor

Yeah that's what I would say as well. I didn't touch Text.cpp. I just applyed the diff.

Contributor

Foaly commented Jul 19, 2014

Yeah that's what I would say as well. I didn't touch Text.cpp. I just applyed the diff.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jul 19, 2014

Member

Ok, fine.

Member

LaurentGomila commented Jul 19, 2014

Ok, fine.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jul 28, 2014

Contributor

It's been more than a week...
What's holding this back?

Contributor

Foaly commented Jul 28, 2014

It's been more than a week...
What's holding this back?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jul 28, 2014

Member

It's summer holidays, people are in vacation or unavailable. Nothing is progressing as we would like.

Be patient, this task is simple to handle (a few more reviews from the team and a merge) and has a high priority, so it shouldn't be long.

Member

LaurentGomila commented Jul 28, 2014

It's summer holidays, people are in vacation or unavailable. Nothing is progressing as we would like.

Be patient, this task is simple to handle (a few more reviews from the team and a merge) and has a high priority, so it shouldn't be long.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 11, 2014

Member

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

Member

eXpl0it3r commented Aug 11, 2014

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

@@ -79,6 +80,9 @@ GlResource::~GlResource()
////////////////////////////////////////////////////////////
void GlResource::ensureGlContext()
{
// Make sure the shared context exists before creating an internal context
assert((Lock(mutex), count));

This comment has been minimized.

@Bromeon

Bromeon Aug 11, 2014

Member

Very strange line. When first looking at it, I came up with the following thoughts:

  • What's the point of the temporary sf::Lock object that locks and immediately unlocks the mutex?
  • Why that strange use of the comma operator?
  • This line has no effect in release mode.
  • The actual condition count != 0 is implicit, making the understanding even more difficult.

After a decent amount of time, I found out that you want to check whether count is non-zero and protect the read access with a mutex, but... please try to avoid such hacks that no one else understands, or at least describe it more clearly in the comment :)

@Bromeon

Bromeon Aug 11, 2014

Member

Very strange line. When first looking at it, I came up with the following thoughts:

  • What's the point of the temporary sf::Lock object that locks and immediately unlocks the mutex?
  • Why that strange use of the comma operator?
  • This line has no effect in release mode.
  • The actual condition count != 0 is implicit, making the understanding even more difficult.

After a decent amount of time, I found out that you want to check whether count is non-zero and protect the read access with a mutex, but... please try to avoid such hacks that no one else understands, or at least describe it more clearly in the comment :)

This comment has been minimized.

@binary1248

binary1248 Aug 11, 2014

Member

This line wasn't 100% necessary. Actually now that I think of it, searching for places where contexts are required from static methods, the only critical places have already been taken care of. We can remove this somewhat strange looking check if you want.

@binary1248

binary1248 Aug 11, 2014

Member

This line wasn't 100% necessary. Actually now that I think of it, searching for places where contexts are required from static methods, the only critical places have already been taken care of. We can remove this somewhat strange looking check if you want.

@@ -522,12 +523,26 @@ void Texture::bind(const Texture* texture, CoordinateType coordinateType)
////////////////////////////////////////////////////////////
unsigned int Texture::getMaximumSize()
{
ensureGlContext();
static unsigned int size = 0;
static bool checked = false;

This comment has been minimized.

@Bromeon

Bromeon Aug 11, 2014

Member

static is not thread-safe. This might be okay given the semantics of sf::Texture, but I want to make sure that you're aware that calling Texture::getMaximumSize() from multiple threads is dangerous. In other places, there seem to be mutexes at least...

@Bromeon

Bromeon Aug 11, 2014

Member

static is not thread-safe. This might be okay given the semantics of sf::Texture, but I want to make sure that you're aware that calling Texture::getMaximumSize() from multiple threads is dangerous. In other places, there seem to be mutexes at least...

This comment has been minimized.

@binary1248

binary1248 Aug 11, 2014

Member

This is only an issue because we are still in C++03 😉. I agree that there might theoretically be problems, but practically? I haven't noticed any myself. The only way to get the variables to be written to (which is the only problem) simultaneously is to call getMaximumSize() for the "first" time from 2 different threads.

I don't know if protecting against this special case is worth it considering it will probably be removed once SFML moves to C++11.

@binary1248

binary1248 Aug 11, 2014

Member

This is only an issue because we are still in C++03 😉. I agree that there might theoretically be problems, but practically? I haven't noticed any myself. The only way to get the variables to be written to (which is the only problem) simultaneously is to call getMaximumSize() for the "first" time from 2 different threads.

I don't know if protecting against this special case is worth it considering it will probably be removed once SFML moves to C++11.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 11, 2014

Member

There are concerns, see code annotations. And has anybody even tested the changes yet?

Member

Bromeon commented Aug 11, 2014

There are concerns, see code annotations. And has anybody even tested the changes yet?

@binary1248 binary1248 self-assigned this Aug 12, 2014

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 13, 2014

Member

The revised version of this PR can be found here:
https://github.com/LaurentGomila/SFML/tree/bugfix/texture_max_size

Member

binary1248 commented Aug 13, 2014

The revised version of this PR can be found here:
https://github.com/LaurentGomila/SFML/tree/bugfix/texture_max_size

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 18, 2014

Member

I've put this again on my merge list. Would be nice if some else can review the updated code as well. If no further concerns get raised it will get merged soon.

Member

eXpl0it3r commented Aug 18, 2014

I've put this again on my merge list. Would be nice if some else can review the updated code as well. If no further concerns get raised it will get merged soon.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 19, 2014

Member

Tested & merged in 7c63c58

Member

eXpl0it3r commented Aug 19, 2014

Tested & merged in 7c63c58

@eXpl0it3r eXpl0it3r closed this Aug 19, 2014

@eXpl0it3r eXpl0it3r modified the milestones: 2.x, 2.2 Aug 19, 2014

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