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

Add method get_enclosing_rect to Geometry2D #7333

Closed
ghost opened this issue Jul 19, 2023 · 7 comments
Closed

Add method get_enclosing_rect to Geometry2D #7333

ghost opened this issue Jul 19, 2023 · 7 comments

Comments

@ghost
Copy link

ghost commented Jul 19, 2023

Describe the project you are working on

N/A

Describe the problem or limitation you are having in your project

Performance issues regarding the loop intensive task to get a Rect2 enclosing a cloud of points.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

It's a common method and easy to implement, but loop intensive.

Geometry2D::get_enclosing_rect can be implemented in the core for performance.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

func get_enclosing_rect(points: PackedVector2Array) -> Rect2:
  if points.is_empty(): return Rect2(0.0, 0.0, 0.0, 0.0)

  var rect := Rect2()

  rect.position = points[0]

  for i in points.size() - 1:
    rect = rect.expand(points[i + 1])

  return rect

If this enhancement will not be used often, can it be worked around with a few lines of script?

Yes.

Is there a reason why this should be core and not an add-on in the asset library?

Performance.

@ghost ghost closed this as not planned Won't fix, can't repro, duplicate, stale Aug 2, 2023
@Zireael07
Copy link

why close?

@ghost
Copy link
Author

ghost commented Aug 3, 2023

why close?

I assumed it was something only wanted by me. but apparently not, I will reopen. 😉

@ghost ghost reopened this Aug 3, 2023
@AThousandShips
Copy link
Member

AThousandShips commented Aug 3, 2023

This is so trivial to work around that I don't think it's worth to add it to core, could document it in Rect2 but it's so trivial that it feels like it should be obvious to people how to do it

However the solution here is inefficient, a better solution is to use min/max, describing how to do that in the documentation could be done, but again as above

I think it would make more sense to add it to Rect2, and add the matching code to AABB, as a constructor from an array possibly, or a static method

Edit: Suggested code for documentation (haven't tested)

func get_enclosing_rect(points: PackedVector2Array) -> Rect2:
    if points.is_empty():
        return Rect2()

    var start := points[0]
    var end := points[0]
    
    for i in range(1, points.size()):
        start = Vector2(min(start.x, points[i].x), min(start.y, points[i].y))
        end = Vector2(max(end.x, points[i].x), max(end.y, points[i].y))

    return Rect2(start, end - start)

The c++ implementation would be something like:

Rect2 get_enclosing_rect(const Vector<Point2> &p_points) {
    if (p_points.is_empty()) {
        return Rect2();
    }
    
    Vector2 start = p_points[0];
    Vector2 end = p_points[0];
    
    for (int i = 1; i < p_points.size(); ++i) {
        start = start.min(p_points[i]);
        end = end.max(p_points[i]);
    }
    
    return Rect2(start, end - start);
}

@Zireael07
Copy link

I think it would make more sense to add it to Rect2, and add the matching code to AABB, as a constructor from an array possibly, or a static method

Either this or document in Rect2

@ghost
Copy link
Author

ghost commented Aug 3, 2023

However the solution here is inefficient, a better solution is to use min/max

Doing it manually won't change much in efficiency, Rect2::expand_to does already "min/max".

@AThousandShips
Copy link
Member

Sure, but this method does it without having to call through that function and just does it directly, and avoids doing subtractions and assignments before being done

@ghost
Copy link
Author

ghost commented Aug 4, 2023

This is so trivial to work around that I don't think it's worth to add it to core, could document it in Rect2 but it's so trivial that it feels like it should be obvious to people how to do it

I also think it's trivial, my intent with the proposal was mainly about the performance gain from letting the C++ side do all the "loop work" (could gain a few milliseconds in heavy loops).

But in reality, if we start doing these kind of things, Geometry2D functions would be endless and I think that's bad.

I'll close the proposal again.

@ghost ghost closed this as not planned Won't fix, can't repro, duplicate, stale Aug 4, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants