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

Rect2().encloses(Rect2()) with same rect is false #26615

Closed
AlexHolly opened this issue Mar 4, 2019 · 6 comments · Fixed by #32478
Closed

Rect2().encloses(Rect2()) with same rect is false #26615

AlexHolly opened this issue Mar 4, 2019 · 6 comments · Fixed by #32478

Comments

@AlexHolly
Copy link
Contributor

Result is false, expected true
Rect2(0,0,1,1).encloses( Rect2(0,0,1,1) )
Rect2(0,0,0,0).encloses( Rect2(0,0,0,0) )

inline bool encloses(const Rect2 &p_rect) const {

Compared with java

import java.awt.Rectangle;

class Main {
  public static void main(String[] args) {
    System.out.println(new Rectangle(0,0,1,1).contains(new Rectangle(0,0,1,1))); // true
    System.out.println(new Rectangle(0,0,0,0).contains(new Rectangle(0,0,0,0))); // false
  }
}

I suggest simply do <= instead of <
https://github.com/godotengine/godot/blob/b16c309f82c77d606472c3c721a1857e323a09e7/core/math/rect2.h#L102-#L103

It will return true in this case which differs from Java but that should be fine.
Rect2(0,0,0,0).encloses( Rect2(0,0,0,0) )

@aaronfranke
Copy link
Member

aaronfranke commented Mar 5, 2019

I suppose the question then would be "what is the desired functionality"? Should a rectangle enclose itself? What are the game programming use cases for true vs false?

See also: #25829 Might want to look over all this code at once.

@ghost
Copy link

ghost commented Jul 25, 2019

if A is a Rect2 representing the game map area
and B is a Rect2 reprensenting the area of a building
then
A.encloses(B) should return true because the workaround is enlarging virtually the map (not really good) or shrinking virtually the futur building (probably by 0.1 map unit) => not very elegant but its what I do because of this bug

@akien-mga
Copy link
Member

@xakz What's your use case for A.encloses(B) if A is the same size as B?

@ghost
Copy link

ghost commented Jul 28, 2019

@akien-mga Probably none but:
Rect2(0, 0, 2, 2).encloses(Rect2(1, 1, 1, 1)) returns false
and
Rect2(0, 0, 2, 2).encloses(Rect2(1, 1, 0.99999, 0.99999)) returns true

For world coordinates, it's not really a problem, but for map coordinates (TileMap) it's not what one expect. We expect a 1x1 tile fit in each corner of a 2x2 tile.

Just some precisons in the doc could be enought.

@fossegutten
Copy link
Contributor

fossegutten commented Aug 5, 2019

Rect2.encloses method is inconsistent now.
When it checks for >= on top and left side, but < on right and bottom sides. Agree with @AlexHolly that it should be changed to <=.
Alternatively > and < on encloses method, and make a new "contains" method, that uses >= and <=.
Edit: typo

@Xrayez
Copy link
Contributor

Xrayez commented Oct 4, 2019

Some geometry methods follow non-strict inequalities already:

<method name="is_point_in_circle">
<return type="bool">
</return>
<argument index="0" name="point" type="Vector2">
</argument>
<argument index="1" name="circle_position" type="Vector2">
</argument>
<argument index="2" name="circle_radius" type="float">
</argument>
<description>
Returns [code]true[/code] if [code]point[/code] is inside the circle or if it's located exactly [i]on[/i] the circle's boundary, otherwise returns [code]false[/code].
</description>
</method>
<method name="is_point_in_polygon">
<return type="bool">
</return>
<argument index="0" name="point" type="Vector2">
</argument>
<argument index="1" name="polygon" type="PoolVector2Array">
</argument>
<description>
Returns [code]true[/code] if [code]point[/code] is inside [code]polygon[/code] or if it's located exactly [i]on[/i] polygon's boundary, otherwise returns [code]false[/code].
</description>
</method>

I've also had to define some constant to emulate strictness (copied directly from my custom camera class to be enclosed within world limits):

const LIMIT_MARGIN = 1.0 # if 0.0 - strictly enclosed

@akien-mga akien-mga added this to the 3.2 milestone Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants