sf::Rect extension

[Tank]

achpile, it's just a pull request — which hasn't been accepted yet.

[ShqrdX]

It would be nice if getCenter also be implemented (rect.left + rect.width/2, rect.top + rect.height/2)

[Tank]

Please see "Why no other corner points" in my post, as it also applies to center points.

[achpile]

It is cannot be done, ShqrdX, because there can be no + or / operators defined for class T

[SeriousITGuy]

Getting position and size as vectors is more convenient to use in my opinion. I'm upvoting this!

[Hapax]

It would be nice if getCenter also be implemented (rect.left + rect.width/2, rect.top + rect.height/2)

(getPosition + getSize() / 2)

isn't simple enough for you? 

Or you could use something like this: Anchor.hpp/Anchor.inl 

[Nexus]

See my earlier post:

This shows already a typical problem of such features: for consistency reasons, they pull many more behind. When there is an accessor for top-left and a bottom-right corner, why not one for top-right and bottom-left? Why no setter for the corresponding parts? What about vertical and horizontal centers?

It can make sense to provide such an API, but it should be more general than for sf::Rect. Getting corner points is useful for other objects as well (sprites, views, windows, ...). Global functions are standing to reason for this. But I'm not sure whether and how this should be part of SFML; I once wanted to introduce something like that in Thor (that's why there is a header called UniformAccess.hpp), but I was not really convinced by the different attempts I've made.

The addition of getPosition() and getSize() can be reasonable with the argument "we only provide what the constructor can set". But in that case we also need to have an explanation why there are no setPosition() and setSize() methods. In my opinion, the constructor + those 2 getters is totally enough.

The advantage over getPosition() and getSize() is clearly the elimination of creating copies all the time.

[and from GitHub:]
I'm just not sure if the user should be warned of the copies the functions create.

We should not overestimate the impact of copying a 2D vector. It's two floats, or basically one double value -- nothing you would even consider worrying about outside of a class. SFML is processing far heavier data (such as sf::Transform) all the time.

And it wouldn't necessarily help if the vectors were members and you returned them by reference -- they're often copied into another object anyway, and if they aren't (and the compiler doesn't optimize the entire call), then the references must also be derefenced, which can be even more costly than copying 64 bit. Without compiler optimizations, I'd worry more about the cost of the function call. And if the compiler optimizes, it's likely that he can optimize this case as well.

tl;dr: premature optimization is evil. A copy being made is obvious from the return type, or DRY as you would say

[Tank]

But in that case we also need to have an explanation why there are no setPosition() and setSize() methods. In my opinion, the constructor + those 2 getters is totally enough.

I'd be fine with adding the set() counter-parts as well, to be honest.

We should not overestimate the impact of copying a 2D vector

Quite true, but we also shouldn't overestimate the impact of using references.

tl;dr: premature optimization is evil. A copy being made is obvious from the return type, or DRY as you would say

I agree to DRY.

However, regarding premature optimization.. This really depends on the perspective now. Because you are prematurely optimizing as well, by NOT using a `const&` for a compound type, which is what one normally does. Just by looking at the trivial case of sf::Vector2 you drop const&. This is clearly an optimization — even a premature one. I took the time to do a little disassembly.

Compiler command used: g++ -O0 -g

Source code:

class Point {
  public:
    Point(float x_, float y_) : x(x_), y(y_) {}
    float x;
    float y;
};

class Compound {
  public:
    Compound() : point(0.0f, 0.0f) {}

    Point get_position_new() { return Point(point.x, point.y); }
    Point get_position_copy() { return point; }
    const Point& get_position_ref() { return point; }

    Point point;
};

int main() {
  Compound foo;

  Point point_new = foo.get_position_new();
  Point point_copy = foo.get_position_copy();
  const Point& point_ref = foo.get_position_ref();

  float x, y;

  x = point_new.x;
  y = point_new.y;

  x = point_copy.x;
  y = point_copy.y;

  x = point_ref.x;
  y = point_ref.y;

  return 0;
}
 

Disassembly output: (using `objdump -d -S -M intel`; only the interesting stuff)

  x = point_new.x;
  4005c4:       f3 0f 10 45 d0          movss  xmm0,DWORD PTR [rbp-0x30]
  4005c9:       f3 0f 11 45 f4          movss  DWORD PTR [rbp-0xc],xmm0
  y = point_new.y;
  4005ce:       f3 0f 10 45 d4          movss  xmm0,DWORD PTR [rbp-0x2c]
  4005d3:       f3 0f 11 45 f0          movss  DWORD PTR [rbp-0x10],xmm0

  x = point_copy.x;
  4005d8:       f3 0f 10 45 c0          movss  xmm0,DWORD PTR [rbp-0x40]
  4005dd:       f3 0f 11 45 f4          movss  DWORD PTR [rbp-0xc],xmm0
  y = point_copy.y;
  4005e2:       f3 0f 10 45 c4          movss  xmm0,DWORD PTR [rbp-0x3c]
  4005e7:       f3 0f 11 45 f0          movss  DWORD PTR [rbp-0x10],xmm0

  x = point_ref.x;
  4005ec:       48 8b 45 f8             mov    rax,QWORD PTR [rbp-0x8]
  4005f0:       f3 0f 10 00             movss  xmm0,DWORD PTR [rax]
  4005f4:       f3 0f 11 45 f4          movss  DWORD PTR [rbp-0xc],xmm0
  y = point_ref.y;
  4005f9:       48 8b 45 f8             mov    rax,QWORD PTR [rbp-0x8]
  4005fd:       f3 0f 10 40 04          movss  xmm0,DWORD PTR [rax+0x4]
  400602:       f3 0f 11 45 f0          movss  DWORD PTR [rbp-0x10],xmm0
 

Conclusion

You have one(!) more MOV instruction for loading the reference address needed for dereferencing `point_ref`. That's about it, not even function calls, and it's the non-optimized case. (I gave up tricking the compiler enough to get reasonable code for disassembly using -O3 ) I don't know of any computer, even from the 90s, that will make a notifiable difference.

Not trying to bash here, really. Actually I wanted to know myself how big the impact is. On a second note though, you played the "Premature optimization" card, and the disassembly proves that moving away from "& for compound types" is a micro optimization and premature.

So yes: Premature optimizations ARE evil and useless.

[Nexus]

I think you over-interpreted what I said. If I say "you shouldn't worry about the copy, it may even be the case that references are slower" that doesn't imply "you should worry about references". My point was not to worry at all

Also, using value return types in such cases is not primarily an optimization because it has additional advantages, such as a simpler interface and less constraints for the implementation (it can change in the future without affecting the API). The "pass compound types by const-reference" was a general rule of thumb, but with improvements in compiler optimizations and at the latest since C++11 move semantics, it has lost quite a bit of its applicability. In C++98, there was a pretty interesting yet controversial article: Want Speed? Pass By Value. A lot has changed since then, but I find it interesting to see how things were never that obvious.

Thanks for the disassembly, by the way

[Tank]

:thumbsup:

[eXpl0it3r]

From an API perspective this is not a very good way to go, but I have heard many people pointing out the usefulness of this, I think we should make a trade off between API "cleanness" and convenience, as in the end users should be using the library.

I think the getters-only implementation is fine for now and if someone can make a strong case for setters, we can still add them.

[Tank]

I vote for both getters and setters. Strong case? 1) I can set position in the ctor, why not through functions? 2) I can get the position and size through functions, why not settings them? 3) I set the values anyway, why not the short and/or logical way?

[Jonny]

Bumping as this is now in 2.6.0 discussion - Any disagreement on adding the setters?

[achpile]

Also i think it would be also good to have function like this:

bool    contains (const Rect< T > &rectangle) const
 

Because now i have to use like this:

if (rect.contains(r.left, r.top) && rect.contains(r.left + r.width, r.top + r.height))
 

or this:

if (rect.intersects(r, intersection) && (r == intersection))