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

[sprite] don't set default rect before the user provided one #1725

Merged
merged 2 commits into from Jan 14, 2021
Merged

[sprite] don't set default rect before the user provided one #1725

merged 2 commits into from Jan 14, 2021

Conversation

vfjpl
Copy link
Contributor

@vfjpl vfjpl commented Dec 8, 2020

Small Optimisation. Using second constructor with user provided rextureRect we first create the default one(by setTexture method) and then overwrite it with the user provided one. With this change we simply first set user provided textureRect and then set the texture which prevents unnecesary creating the default one.

Please review.

Cheers.

@eXpl0it3r
Copy link
Member

eXpl0it3r commented Dec 8, 2020

Can you provide an example that explains the use case here?

Also have you seen the code in setTexture?

// Recompute the texture area if requested, or if there was no valid texture & rect before
if (resetRect || (!m_texture && (m_textureRect == sf::IntRect())))
setTextureRect(IntRect(0, 0, texture.getSize().x, texture.getSize().y));

@vfjpl
Copy link
Contributor Author

vfjpl commented Dec 8, 2020

Also have you seen the code in setTexture? yes, this is why I did this.

Exapmple is simple
sf::Sprite newSprite(texture, userRectangle);

Before sprite would create internal rectangle based on texture size, set texture, and then create new internal rectangle based on userRectangle.
Now it create new internal rectangle based on userRectangle and then sets the texture.

@eXpl0it3r
Copy link
Member

Ah okay, that seems to make sense to me then. Somehow had a hard time understanding the purpose. 😄

@LaurentGomila any reason why we didn't do it this way from the beginning?

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Dec 8, 2020
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Dec 8, 2020
@eXpl0it3r eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Dec 8, 2020
@LaurentGomila
Copy link
Member

I don't really remember. Just a guess: the resetRect argument may have been added after the constructors, and we didn't really thought about this optimization.

@vfjpl
Copy link
Contributor Author

vfjpl commented Dec 9, 2020

@eXpl0it3r @LaurentGomila Potential problem seems to be when the user want to set rect to sf::IntRect(0, 0, 0, 0)
Like:
sf::Sprite newSprite(texture, sf::IntRect(0, 0, 0, 0));

before:
sprite would create internal rectangle based on texture size, set texture, and then create new internal rectangle based on userRectangle.
now:
sprite would create new internal rectangle based on userRectangle don't set any internal rectangle because it's already "0"

if (rectangle != m_textureRect)
, set texture and then create internal rectangle based on texture size (because rectangle the internal rectangle is all "0").

I'm not sure if there are any legitimate usages for setting rect to all 0 but it's something I didn't thought of yesterday.
Maybe it would be a bug in user code so we have "saved" him?

edit:
anyway if we wan't to allow user setting rect to sf::IntRect(0, 0, 0, 0) then simply changle line

setTexture(texture, false);

to m_texture = &texture;

SFML 2.6.0 automation moved this from Review & Testing to Ready Jan 6, 2021
@eXpl0it3r eXpl0it3r merged commit b939c79 into SFML:master Jan 14, 2021
SFML 2.6.0 automation moved this from Ready to Done Jan 14, 2021
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

4 participants