Outline vertices are only updated if there is an outline. #925

tylerellis21 · 2015-07-15T03:35:47Z

SFML Tasks

Create new branch
Add optimization back in
Create new pull request

Improved the logic of the shape class so that the outline vertices are only updated if they exist.

tylerellis21 · 2015-07-15T04:12:32Z

Also I have been doing some testing on the change it seems to help a lot on my system.
I have cut out some of the undeeded methods to show the Shape::updateOutline since that was my focus and the only difference between the two tests was my change.
The tool used for the performance test was: http://oprofile.sourceforge.net/
Ran on Ubuntu 14.04.

https://gist.github.com/tylerellis21/f9a7663262feaf150a83

I ran the program for the same amount of time but the simulation wasn't 100% the same, but the results do speak for themselves.

Bromeon · 2015-07-15T06:59:46Z

Thanks for the contribution. For the future, please read the contribution guidelines before just creating pull requests out of the blue.

Your code makes several things worse (the sf:: prefix and many comments that don't add any information), and the case differentiation in Shape::updateOutline is very convoluted. A simple

// Return if there is no outline to build
if (outlineThickness == 0.f)
{
    m_outlineVertices.clear();
    m_bounds = m_insideBounds;
    return;
}

before anything else would have been much clearer. Obvious comments like

m_outlineVertices.clear(); // Clear out any vertices that may remain from a previous outline size 
return; // Return since there is no outline to build

don't add any useful information, omit them entirely.

Furthermore, if there is a check for zero outline thickness when creating the outline, there needn't be another one when drawing.

Since this is not a bugfix but an optimization, it should target master rather than the 2.3.x branch, which is for critical patches (you can't change that, but I'll write it as a note to the one who may merge it).

If you want to prove anything with your performance test, you should show us the code. Ideally, it is minimal (a single main() function) and contains nothing that's not SFML-related.

tylerellis21 · 2015-07-16T18:02:57Z

Sorry about the out of the blue push. I'm sorry I didn't see the contribution guide first and from now on I will definitely follow it down the the dot. I changed my logic so it's not as confusing, but I was wondering on the check for the outline draw. It sets the renderstates texture to null if there is an outline in that check. If that is removed would that cause any adverse affects? Thanks again and sorry for the inconvenience.

zsbzsb · 2015-07-16T21:52:29Z

src/SFML/Graphics/Shape.cpp

@@ -204,7 +204,6 @@ void Shape::update()
    // Texture coordinates
    updateTexCoords();

-    // Outline


Why was this comment removed?

It seemed kinda redundant. Since the method it self is named updateOutline, but I can undo that If needed.

True, but now it's inconsistent, because there's a comment above updateFillColors() and updateTexCoords(), but not above updateOutline(). In general, I'd focus more on the issue and less on all the other existing code 😉

zsbzsb · 2015-07-16T21:57:46Z

It sets the renderstates texture to null if there is an outline in that check. If that is removed would that cause any adverse affects?

You mean if the texture assignment to null was removed? If that was removed then the user would see their texture being drawn inside the outline. I'm not even sure why you would want that assignment to be removed.

tylerellis21 · 2015-07-17T00:15:35Z

I was talking about what Bromeon said "Furthermore, if there is a check for zero outline thickness when creating the outline, there needn't be another one when drawing". Was just trying to clarify that. It seems that we would still want that.

Bromeon · 2015-07-17T06:13:04Z

but I was wondering on the check for the outline draw. It sets the renderstates texture to null if there is an outline in that check. If that is removed would that cause any adverse affects?

This is the code:

    // Render the outline
    if (m_outlineThickness != 0.f)
    {
        states.texture = NULL;
        target.draw(m_outlineVertices, states);
    }

states is only a parameter, the texture is reset so that the outline is not textured. If m_outlineVertices is empty, nothing will be done, see here. There may however be additional virtual function calls; leaving the if statement is probably cheaper -- but then it's purely an optimization and should be commented as such.

eXpl0it3r · 2015-09-20T21:47:59Z

Can someone review this?

Bromeon · 2015-09-20T21:52:15Z

src/SFML/Graphics/Shape.cpp

@@ -251,7 +248,16 @@ void Shape::updateTexCoords()
 ////////////////////////////////////////////////////////////
 void Shape::updateOutline()
 {
+    // Return if there is no outline to build.
+    if (m_outlineThickness != 0.f)


Shouldn't that be ==?

A single commit would also be nice.

Bromeon · 2015-10-02T09:55:41Z

What's the state of this? @tylerellis21, are you still willing to update the PR, or should somebody else take care of it?

Also, further reviews would be nice.

tylerellis21 · 2015-10-02T23:19:42Z

@Bromeon To be honest, I forgot about this PR, but I am willing to update it. I will get on it tonight and fix my dumb mistakes!

zsbzsb · 2015-10-03T13:25:57Z

src/SFML/Graphics/Shape.cpp

    std::size_t count = m_vertices.getVertexCount() - 2;
+


Do we also need a new blank line here with no new surrounding code?

zsbzsb · 2015-10-03T21:01:45Z

Other than the thing @Bromeon suggested and what I mentioned (new blank line) the PR looks good to me 👍

tylerellis21 · 2015-10-04T00:46:40Z

In theory I squashed the commits into one. I also updated the logic to be correct and removed that extra line. I have never done a squash before, so I just want to make sure it looks good to you guys. Thanks everyone!

zsbzsb · 2015-10-04T02:47:18Z

Looks great 👍 (much better squishing than what I have seen some people do 😉)

binary1248 · 2015-10-10T00:38:29Z

I would also leave in the optimization that @Bromeon was talking about. If it doesn't break anything, there is no need to get rid of it if it is already part of the existing code.

eXpl0it3r · 2015-10-25T21:03:10Z

So should the if be added back again?

Bromeon · 2015-10-25T22:37:00Z

It makes the code a bit more difficult to understand, and it's not obvious why the line thickness is checked in two places, when one is enough. If we add it, I would at least write an expressive comment to state that it's a pure micro-optimization and not a behavioral necessity.

eXpl0it3r · 2015-11-10T08:10:02Z

So should we? Should we not? Do we need a poll?

Bromeon · 2015-11-10T12:29:54Z

I stated what we can do in my opinion:

Omit the micro-optimization
Leave it with an expressive comment

I.e. I don't care. I really hope we don't need a poll for every tiny decision... :)

binary1248 · 2016-05-08T21:25:34Z

Bump. Still for keeping the diff minimal.

eXpl0it3r · 2016-06-22T19:52:17Z

Any takers? Do we need a poll?

eXpl0it3r · 2016-10-13T22:36:35Z

@tylerellis21 Can you add the optimization back in, so we can get this PR finally merged? 😄

eXpl0it3r · 2016-11-08T07:46:31Z

@tylerellis21 Bump! Needs updating and as mentioned adjusting.

eXpl0it3r · 2016-11-08T16:51:40Z

Did you intentionally close this and deleted your branch of it, @tylerellis21?

Foaly · 2018-01-26T12:19:24Z

Should I write a new PR for this?

eXpl0it3r · 2018-01-26T12:38:05Z

Please do so! 🙂

eXpl0it3r · 2018-01-26T14:49:10Z

Superseded by #1356

Bromeon added this to the 2.4 milestone Jul 15, 2015

Bromeon added feature m:sfml-graphics s:undecided labels Jul 15, 2015

zsbzsb reviewed Jul 16, 2015
View reviewed changes

Bromeon reviewed Sep 20, 2015
View reviewed changes

zsbzsb reviewed Oct 3, 2015
View reviewed changes

Update outline vertices only if there is an outline.

61dcba3

tylerellis21 force-pushed the 2.3.x branch from 882e93b to 61dcba3 Compare October 4, 2015 00:39

eXpl0it3r modified the milestones: 2.5, 2.4 Jul 7, 2016

Merge branch '2.3.x' into 2.3.x

1a3af51

tylerellis21 closed this Nov 8, 2016

tylerellis21 deleted the 2.3.x branch November 8, 2016 08:59

Foaly mentioned this pull request Jan 26, 2018

Generate shape outline vertices only if necessary. #1356

Merged

eXpl0it3r added s:superseded and removed s:undecided labels Jan 26, 2018

eXpl0it3r moved this from WIP to Merged / Superseded in SFML 2.5.0 Jan 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Outline vertices are only updated if there is an outline. #925

Outline vertices are only updated if there is an outline. #925

tylerellis21 commented Jul 15, 2015 •

edited by eXpl0it3r

tylerellis21 commented Jul 15, 2015

Bromeon commented Jul 15, 2015

tylerellis21 commented Jul 16, 2015

zsbzsb Jul 16, 2015

tylerellis21 Jul 17, 2015

Bromeon Jul 17, 2015

zsbzsb commented Jul 16, 2015

tylerellis21 commented Jul 17, 2015

Bromeon commented Jul 17, 2015

eXpl0it3r commented Sep 20, 2015

Bromeon Sep 20, 2015

Bromeon commented Oct 2, 2015

tylerellis21 commented Oct 2, 2015

zsbzsb Oct 3, 2015

Bromeon Oct 3, 2015

zsbzsb commented Oct 3, 2015

tylerellis21 commented Oct 4, 2015

zsbzsb commented Oct 4, 2015

binary1248 commented Oct 10, 2015

eXpl0it3r commented Oct 25, 2015

Bromeon commented Oct 25, 2015

eXpl0it3r commented Nov 10, 2015

Bromeon commented Nov 10, 2015

binary1248 commented May 8, 2016

eXpl0it3r commented Jun 22, 2016

eXpl0it3r commented Oct 13, 2016

eXpl0it3r commented Nov 8, 2016

eXpl0it3r commented Nov 8, 2016

Foaly commented Jan 26, 2018

eXpl0it3r commented Jan 26, 2018

eXpl0it3r commented Jan 26, 2018

Outline vertices are only updated if there is an outline. #925

Outline vertices are only updated if there is an outline. #925

Conversation

tylerellis21 commented Jul 15, 2015 • edited by eXpl0it3r

tylerellis21 commented Jul 15, 2015

Bromeon commented Jul 15, 2015

tylerellis21 commented Jul 16, 2015

zsbzsb Jul 16, 2015

Choose a reason for hiding this comment

tylerellis21 Jul 17, 2015

Choose a reason for hiding this comment

Bromeon Jul 17, 2015

Choose a reason for hiding this comment

zsbzsb commented Jul 16, 2015

tylerellis21 commented Jul 17, 2015

Bromeon commented Jul 17, 2015

eXpl0it3r commented Sep 20, 2015

Bromeon Sep 20, 2015

Choose a reason for hiding this comment

Bromeon commented Oct 2, 2015

tylerellis21 commented Oct 2, 2015

zsbzsb Oct 3, 2015

Choose a reason for hiding this comment

Bromeon Oct 3, 2015

Choose a reason for hiding this comment

zsbzsb commented Oct 3, 2015

tylerellis21 commented Oct 4, 2015

zsbzsb commented Oct 4, 2015

binary1248 commented Oct 10, 2015

eXpl0it3r commented Oct 25, 2015

Bromeon commented Oct 25, 2015

eXpl0it3r commented Nov 10, 2015

Bromeon commented Nov 10, 2015

binary1248 commented May 8, 2016

eXpl0it3r commented Jun 22, 2016

eXpl0it3r commented Oct 13, 2016

eXpl0it3r commented Nov 8, 2016

eXpl0it3r commented Nov 8, 2016

Foaly commented Jan 26, 2018

eXpl0it3r commented Jan 26, 2018

eXpl0it3r commented Jan 26, 2018

tylerellis21 commented Jul 15, 2015 •

edited by eXpl0it3r