SFML community forums

General => Feature requests => Topic started by: achpile on August 28, 2014, 03:04:53 pm

Title: sf::Rect extension
Post by: achpile on August 28, 2014, 03:04:53 pm
I think it will be useful functions to get position, size and (position + size) of the rectangle class instead of using everywhere in the code sf::Vector2(rect.left, rect.top) use simple rect.getPosition().

I don't want to make a pull request, 'cause some my things (like doxygen comments and formatting) may differ of what supposed to be. So here's a simple patch. And I don't know, how to name function returning (position + size) :D

diff --git a/include/SFML/Graphics/Rect.hpp b/include/SFML/Graphics/Rect.hpp
index 2880722..d565b5e 100644
--- a/include/SFML/Graphics/Rect.hpp
+++ b/include/SFML/Graphics/Rect.hpp
@@ -146,6 +146,36 @@ public :
     bool intersects(const Rect<T>& rectangle, Rect<T>& intersection) const;
 
     ////////////////////////////////////////////////////////////
+    /// \brief Get the position of the rectangle
+    ///
+    /// This returns the position of the rectangle
+    ///
+    /// \return Rectangle position
+    ///
+    ////////////////////////////////////////////////////////////
+    Vector2<T> getPosition();
+
+    ////////////////////////////////////////////////////////////
+    /// \brief Get the size of the rectangle
+    ///
+    /// This returns the size of the rectangle
+    ///
+    /// \return Rectangle size
+    ///
+    ////////////////////////////////////////////////////////////
+    Vector2<T> getSize();
+
+    ////////////////////////////////////////////////////////////
+    /// \brief Get the position of the far point of the rectangle
+    ///
+    /// This returns the position of the far point of the rectangle
+    ///
+    /// \return Rectangle position of the far point
+    ///
+    ////////////////////////////////////////////////////////////
+    Vector2<T> getPositionFar();
+
+    ////////////////////////////////////////////////////////////
     // Member data
     ////////////////////////////////////////////////////////////
     T left;   ///< Left coordinate of the rectangle
diff --git a/include/SFML/Graphics/Rect.inl b/include/SFML/Graphics/Rect.inl
index 4a92397..779c904 100644
--- a/include/SFML/Graphics/Rect.inl
+++ b/include/SFML/Graphics/Rect.inl
@@ -157,3 +157,27 @@ inline bool operator !=(const Rect<T>& left, const Rect<T>& right)
 {
     return !(left == right);
 }
+
+
+////////////////////////////////////////////////////////////
+template <typename T>
+Vector2<T> Rect<T>::getPosition()
+{
+    return Vector2<T>(left, top);
+}
+
+
+////////////////////////////////////////////////////////////
+template <typename T>
+Vector2<T> Rect<T>::getSize()
+{
+    return Vector2<T>(width, height);
+}
+
+
+////////////////////////////////////////////////////////////
+template <typename T>
+Vector2<T> Rect<T>::getPositionFar()
+{
+    return Vector2<T>(static_cast<T>(left + width), static_cast<T>(top + height));
+}
 
Title: Re: sf::Rect extension
Post by: eXpl0it3r on September 19, 2014, 11:44:58 pm
Let me bump this thread, so others from the SFML Team get another chance to comment. :P
Title: Re: sf::Rect extension
Post by: Hiura on September 20, 2014, 10:29:02 am
What is the "position" of a rectangle? Its top left corner, its center, its bottom right corner, ...? It's not clear and people might have different answer to that question. So if we really want a function to return a specific point of the rectangle, it should not be called «getPosition» IMO. Same apply for the «far position».

And then, if we have `getTopLeftPoint()`, we should also get the 4 other important points of the rectangle with a similar method for consistency matter. But I feel it would just bloat the API for pretty much nothing.

Since anyone can implement those function according to their specific needs with free function, I'd rather not add those 6 functions in the API. Would you agree?
Title: Re: sf::Rect extension
Post by: Cornstalks on September 20, 2014, 05:44:55 pm
What is the "position" of a rectangle? Its top left corner, its center, its bottom right corner, ...? It's not clear and people might have different answer to that question. So if we really want a function to return a specific point of the rectangle, it should not be called «getPosition» IMO. Same apply for the «far position».
Presumably, "position" is exactly what it is in the constructor "Rect (const Vector2< T > &position, const Vector2< T > &size)". I get what you're saying, but reality is the Rect already has a sense of "position" (which, from the docs, is "Position of the top-left corner of the rectangle"). I think if you don't want to have a notion of "position" then that constructor ought to be removed. If that constructor stays, though, then Rects should have a proper sense of position.

Personally, I think it might be useful to have getPosition() and getSize() methods that return Vector2. I know you can just access the left/top/width/height members, but being able to concisely get a Vector2 that's consistent with an existing constructor would be useful. I don't really like getPositionFar().
Title: Re: sf::Rect extension
Post by: Hiura on September 20, 2014, 07:20:41 pm
Quote
Presumably, "position" is exactly what it is in the constructor "Rect (const Vector2< T > &position, const Vector2< T > &size)".
Thanks for mentioning it. I think we should rename this parameter to `topLeftPoint` or something.
Title: Re: sf::Rect extension
Post by: Hapax on September 20, 2014, 07:57:41 pm
getPositionFar() sounds horrible  :P
When I first read this, I thought that getBottomRight() would be better and then Hiura brings up that the "position" should be topLeft. I think we're onto a winner here...  ;)
Title: Re: sf::Rect extension
Post by: Nexus on September 20, 2014, 09:15:14 pm
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.
Title: Re: sf::Rect extension
Post by: Hapax on September 21, 2014, 01:35:22 am
Rect already returns the top-left; it's just called position at the moment.
Top-left and bottom-right are common, as are top-left and size. Bottom-right and size can obviously easily converted back and forth, of course.
Rect is the more general API. Sprites and views already return rects so it would automatically be applied to those too.
Title: Re: sf::Rect extension
Post by: Nexus on September 21, 2014, 02:13:32 am
Rect already returns the top-left; it's just called position at the moment.
No, there's merely a constructor from position and size, and it only exists for convenience as an alternative to the constructor taking 4 coordinates. sf::Rect has no getter functions, just 4 public members.

Rect is the more general API. Sprites and views already return rects so it would automatically be applied to those too.
sf::Rect can't represent rotated rectangles. And even then, it's not possible to implement something like
void setLeft(sf::Sprite& sprite, float x);
via sf::Rect -- sf::Sprite::setPosition() needs to be called directly.

But as mentioned, I implemented this for some cases, and there are a few non-trivial design decisions if you want to keep it truly generic. Ideally, a user could provide a small set of global functions for his own classes, and then all these accessors like setLeft(), getRightBottom(), getCenter(), ... would work automatically. But this is something that can be implemented on top of SFML -- and there should be a well-designed version of it before even considering the integration into SFML itself.

A sf::Rect::getBottomRight() method is not completely thought through. A simple example is the case where the rectangle has negative width and height (an allowed scenario, e.g. for mirrored sprites) -- now the term "bottom-right" is suddenly lying, as it refers to the top-left corner. And it's not simply fixed by a case differentiation, because no user would expect that. The next step would be "if there is getBottomRight(), there should also be setBottomRight()", which comes not only with the previous problem, but can be implemented with different policies: maintain the size and move the rectangle, or resize the rectangle.

To sum up, additions like a getBottomRight() always sound simple at first glance, but you need to think about the long-term implications on the API whenever you consider adding functions. Often, it's better to keep APIs small and intuitive, especially when they provide all the necessary means to write more powerful functionality on top of them.
Title: Re: sf::Rect extension
Post by: Hapax on September 21, 2014, 06:33:52 am
No, there's merely a constructor from position and size, and it only exists for convenience as an alternative to the constructor taking 4 coordinates. sf::Rect has no getter functions, just 4 public members.
Ah, you're right, of course. It doesn't return top and left as a combined type. Still, the "position" being rename topLeft would be a bit more accurate.

I say a "bit" more accurate due to the rest of your post about mirrored sizes. It's all very interesting and, of course, correct. It's true that if the sizes are negative then the corners would no longer be named correctly. To counter this, "position" should be "initialCorner" or "startCorner".
However, the actual member names of Rect do not take into account the possibility of this negative sizing, with them being labelled as left and top. If the width and/or height are negative, left and top become right and bottom, depending on which are negative. I do not find this a problem as most of us would "read" a rectangle as "first corner to second corner" and automatically associate left and top with first corner. This brings us full circle where topLeft and bottomRight start looking okay again.

Strangely enough, though, I think I am playing Devil's advocate, as I don't see any "need" to provide so-called bottom-right location as it's easy enough to calculate using the members.

I do think the original "get vector" idea makes quite a bit of sense but could easily be coded on top of SFML.

That said, startCorner and endCorner make sense as locations if you're taking into account the mirroring. So, instead of "position", it should really be "startCorner", "startPosition" or just "start".

getStart() and getSize() seems reasonably useful functions.
getEnd() is just getStart() + getSize() since the two vectors can be added together  8)
Title: Re: sf::Rect extension
Post by: Cornstalks on September 21, 2014, 09:56:36 pm
I think "position" and "size" are good. No need for the ability to get all the corners.

I might be the only one who wants this, but I feel like the notion of top/left (or any other specific corner) should be banished. Allow rects with a negative width/height. Then, "position" is just the anchor point for the rect, which idiomatically is the top-left corner, but no longer forces the notion of top/left, and then the user is free to make the anchor point whatever makes sense for his/her program (want a bottom-left anchor point? great! just use a negative height).

I think it's simple to say the position is the anchor (heck, I'd even be okay if it was called "anchor" instead, though "position" is fine too). A rectangle is defined by points in the range [position, position + size].

I don't think it's inconsistent to provide a position/anchor point and a size (and nothing else).

edit: I just read Hapax's comment above. I like most of what he said, though I don't like the names (getStart/getSize/getEnd). I'd personally prefer something like "anchor" and "size". While a member dedicated to getting (anchor + size) would be useful, I can't think of a name that I like for it, and I'm the kind of person that would rather not have something if it's named something that feels... weird.
Title: Re: sf::Rect extension
Post by: Hapax on September 22, 2014, 12:05:08 am
I don't like "anchor"; sorry  :P
It would be more consistent to call it "origin", maybe.
That said, "position" is still fine with me. I, personally, don't really have a problem with it. It's called that in the constructor; it's fine for the "getter".

Names aside, I think the point is really just "do we really need vector returning functions for these values or should we write our own functions to create them from its components (or just apply them manually)?"
I'm sort of on the fence with this one, though I think I'm leaning more towards the side of having them. It makes sense to be able to have a function to get the same types we used to construct the object with.
Title: Re: sf::Rect extension
Post by: wintertime on September 22, 2014, 12:58:11 pm
A while ago I replaced a use of sf::Rect in my code with my own that contains 2 vectors. I think its more consistent that way, so there is no more need to manually access both x and y or to look if, for example, one y value was called top, up or height.
Title: Re: sf::Rect extension
Post by: Tank on January 25, 2016, 11:48:39 am
Made a pull request for getPosition() and getSize(): https://github.com/SFML/SFML/pull/1047

Part 1:

Why no other corner points: Because I wanted simple compound wrappers for the members that are already there, namely left/top, width/height. Other corners would contain logic and require calculations. This can be discussed, but it's not what I intended. It can even be discussed completely separated from this.

Why "position" and "size" as names: Because that's how SFML represents the rectangle as a concept. sf::Rect already takes "position" and "size" in one variant of a constructor, so this concept can be used further. And to be honest, it's totally well-known everywhere else: Where is the rect, and how large is it? Since we have a +x/-y coordinate system, expansion logic is also natural.

Part 2, for SFML 3:

Replace left/top by "sf::Vector2 position" and width/height by "sf::Vector2 size".

The main reason being, like in part 1, ease of usage ("rect.position + rect.size / 2" to get the center vs. "sf::Vector2f(rect.left, rect.top) + sf::Vector2f(rect.width, rect.height) / 2.0f"). The advantage over getPosition() and getSize() is clearly the elimination of creating copies all the time.
Title: Re: sf::Rect extension
Post by: achpile on January 25, 2016, 11:54:16 am
Wow, nice to see my suggestion going to be part of SFML  ;) Thanks Tank!
Title: Re: sf::Rect extension
Post by: Tank on January 25, 2016, 11:58:05 am
achpile, it's just a pull request — which hasn't been accepted yet. ;)
Title: Re: sf::Rect extension
Post by: ShqrdX on January 25, 2016, 07:52:46 pm
It would be nice if getCenter also be implemented (rect.left + rect.width/2, rect.top + rect.height/2)
Title: Re: sf::Rect extension
Post by: Tank on January 26, 2016, 07:47:32 am
Please see "Why no other corner points" in my post, as it also applies to center points.
Title: Re: sf::Rect extension
Post by: achpile on January 26, 2016, 08:29:28 am
It is cannot be done, ShqrdX, because there can be no + or / operators defined for class T
Title: Re: sf::Rect extension
Post by: SeriousITGuy on January 26, 2016, 10:16:18 am
Getting position and size as vectors is more convenient to use in my opinion. I'm upvoting this!
Title: Re: sf::Rect extension
Post by: Hapax on January 27, 2016, 01:07:23 am
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?  :P

Or you could use something like this: Anchor.hpp (https://github.com/Hapaxia/Plinth/blob/master/Plinth/Sfml/Anchor.hpp)/Anchor.inl (https://github.com/Hapaxia/Plinth/blob/master/Plinth/Sfml/Anchor.inl)  ;)
Title: Re: sf::Rect extension
Post by: Nexus on January 28, 2016, 04:37:15 pm
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 ;)
Title: Re: sf::Rect extension
Post by: Tank on January 29, 2016, 09:08:51 am
Quote from: Nexus
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.

Quote from: Nexus
We should not overestimate the impact of copying a 2D vector
Quite true, but we also shouldn't overestimate the impact of using references.

Quote from: Nexus
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. :D

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 :D) 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. :D
Title: Re: sf::Rect extension
Post by: Nexus on January 29, 2016, 08:02:30 pm
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. (http://web.archive.org/web/20140205194657/http://cpp-next.com/archive/2009/08/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 :)
Title: Re: sf::Rect extension
Post by: Tank on January 30, 2016, 09:12:17 am
:thumbsup: ;)
Title: Re: sf::Rect extension
Post by: eXpl0it3r on November 20, 2017, 05:13:23 pm
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.
Title: Re: sf::Rect extension
Post by: Tank on November 21, 2017, 08:25:47 am
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?
Title: Re: sf::Rect extension
Post by: Jonny on August 14, 2018, 05:31:51 pm
Bumping as this is now in 2.6.0 discussion - Any disagreement on adding the setters?
Title: Re: sf::Rect extension
Post by: achpile on September 07, 2018, 10:57:09 am
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))