Reimplement Rect2 functions #242

juliohq · 2023-04-23T18:07:54Z

Reimplement Rect2 functions as proposed by #209. It's missing to reimplement is_infinite() somehow, since it needs an inf type.

lilizoey

Overall the implementation mostly looks good!

A couple of things though.

Rect2 is a Copy type, so it'd make sense for a lot of these functions to take Self by value instead of by reference. We dont have strict guidelines for this but generally i'd say that any method that takes in a Rect2 and returns a transformed Rect2 should take Self by value.

Regarding is_finite, i dont think you need a new type for this? It should just be a call to Vector2::is_finite on position and size.

Additionally you should add tests. Try to test what you can in rect2, and create integration tests to ensure that our implementation does the same as Godot's implementation. Take a look at rect2i_test.rs for integration tests.

Also could you uncomment the assert_nonnegative function? As you've implemented Rect2::abs().

godot-core/src/builtin/rect2.rs

Bromeon

To me, it's not clear why some parts reuse the existing Rect2i implementation, while others are seemingly written from scratch.

For Rect2i, we have done this exact exercise already, and now we're on a path to provide two different implementations for the same thing 🙂

I think instead of bikeshedding the current code, we should focus on discussing reuse strategies (e.g. a macro like with vectors). Or at least align the implementation in a way that makes such an abstraction straightforward.

Bromeon · 2023-04-24T14:33:37Z

godot-core/src/builtin/rect2.rs

@@ -44,6 +44,184 @@ impl Rect2 {
        }
    }

+    /// Returns a rectangle with equivalent position and area, modified so that the top-left corner is the origin and `width` and `height` are positive.


This is confusing:

position can be different, not equivalent (I know what you mean, but in this context it is assumed to refer to the field).

width/height are not positive, but non-negative

I'd keep it simple, something like:

Returns a rectangle with the same geometry, with top-left corner as position and non-negative size.

I just copied the same contents you can find in the official docs (https://docs.godotengine.org/en/stable/classes/class_rect2.html#method-descriptions). Do you think I should really change that?

Yes, the official documentation is often confusing or under-specified.

If we can do better, there's no reason to copy bad docs.

godot-core/src/builtin/rect2.rs

Bromeon · 2023-04-24T17:30:49Z

I'm still missing an answer to my last comment 😉

juliohq · 2023-04-26T11:06:39Z

What do you think about implementing a RectCommons trait with type target = X; for the target numeric value (i.e. real/i64)? That way we'll have a simplified set of already implemented functions that should be reused by both Rect structs. I'm not sure if that would work for every case though and thus some functions might still have to be implemented for each specific struct.

lilizoey · 2023-04-26T15:55:03Z

What do you think about implementing a RectCommons trait with type target = X; for the target numeric value (i.e. real/i64)? That way we'll have a simplified set of already implemented functions that should be reused by both Rect structs. I'm not sure if that would work for every case though and thus some functions might still have to be implemented for each specific struct.

One issue with this is that you can't make functions const for Rect2i. And in the documentation these functions would show up as a trait implementation instead of as inherent method. Also i feel like a macro would probably work better here, i doubt we're gonna be using this trait to do anything other than just implementing the methods.

Do you have a use-case where it being a trait would be useful beyond just as a way to implement the methods?

juliohq · 2023-04-26T17:18:54Z

One issue with this is that you can't make functions const for Rect2i.

Do you mean it's a limitation of Rust (as proposed here rust-lang/rust#71971) or a limitation of the current design for Rect2i? If so, yes I think a macro would be more helpful in this case.

lilizoey · 2023-04-26T18:16:48Z

One issue with this is that you can't make functions const for Rect2i.

Do you mean it's a limitation of Rust (as proposed here rust-lang/rust#71971) or a limitation of the current design for Rect2i? If so, yes I think a macro would be more helpful in this case.

I mean it's a limitation of rust yes, we wouldn't be able to implement functions that are identical for Rect2i and Rect2 as const fn for Rect2i if we do it through a trait.

Bromeon · 2023-04-26T19:38:52Z

I agree that macro would probably fit better here. It seems to work nicely with the vector types.

It's btw also the approach that glam uses to implement is various vector types, and the API is much clearer than for overengineered generic ones like euclid.

Bromeon

Thanks! Added a few more comments 🙂

Unifying this with Rect2i can be done separately at a later stage.

Could you squash the commits to 1 in the end?

itest/rust/src/rect2_test.rs

Bromeon · 2023-05-08T20:45:29Z

itest/rust/src/rect2_test.rs

+        for b in test_reals {
+            evaluate_mappings("grow", a.grow(b as f32), inner_a.grow(b));
+
+            for c in test_reals {
+                for d in test_reals {
+                    for e in test_reals {
+                        evaluate_mappings(
+                            "grow_individual",
+                            a.grow_individual(b as f32, c as f32, d as f32, e as f32),
+                            inner_a.grow_individual(b, c, d, e),
+                        );
+                    }
+                }
+            }


This runs 4^4 = 256 iterations. If someone adds values to test_reals, the time complexity explodes.

It's probably OK to have a 4-depth nested loop to test all combinations (as long as the number is bounded), but it might be more interesting to test some other values. For example include negative ones.

Maybe instead of test_reals, define a separate

let grow_values = [-1.0, 0.0, 1.0, 7.0];

that also ensures that people only change this when they actually want to affect the testing of grow().

Just getting precision errors like that:

ERROR: Panic msg: assertion failed: `(left == right)` left: `Rect2 { position: Vector2 { x: 1.2, y: -0.7 }, size: Vector2 { x: -0.5, y: 1.9000001 } }`, right: `Rect2 { position: Vector2 { x: 1.2, y: -0.7 }, size: Vector2 { x: -0.5, y: 1.9 } }`: grow_individual: outer != inner (Rect2 { position: Vector2 { x: 1.2, y: -0.7 }, size: Vector2 { x: -0.5, y: 1.9000001 } } != Rect2 { position: Vector2 { x: 1.2, y: -0.7 }, size: Vector2 { x: -0.5, y: 1.9 } })

How do you think we could fix this?

We have a function is_equal_approx and a macro assert_eq_approx!. You can pass the function to the macro.

The is_equal_approx also exists on Vector2, so you don't need to apply it to individual coordinates.

@juliohq what do you think about my suggestion? 🙂
It's one of the last things before this PR should be ready!

@Bromeon I wonder if there's any way to replace the logic in evaluate_mappings with is_equal_approx or assert_eq_approx! while keeping the inner and outer parameters.

You could use my suggestion from here, and call assert_eq_approx! inside; or am I misunderstanding you?

I mean, the current evaluate_mappings uses assert_eq!, which takes a custom panic message and thus we can print the outer and inner variables, I'm not sure if is_equal_approx or assert_eq_approx! does the same.

They do, even if it's not called "outer" and "inner" but "left" and "right". That's probably OK.

gdext/godot-core/src/builtin/math.rs

Lines 176 to 194 in 65ce2c3

/// Asserts that two values are approximately equal, using the provided `func`

/// for equality checking.

#[macro_export]

macro_rules! assert_eq_approx {

($a:expr, $b:expr, $func:expr $(,)?) => {

match ($a, $b) {

(a, b) => {

assert!(($func)(a,b), "\n left: {:?},\n right: {:?}", $a, $b);

}

}

};

($a:expr, $b:expr, $func:expr, $($t:tt)+) => {

match ($a, $b) {

(a, b) => {

assert!(($func)(a,b), "\n left: {:?},\n right: {:?},\n{}", $a, $b, format_args!($($t)+));

}

}

};

}

GodotRust · 2023-05-19T16:54:39Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-242

Bromeon · 2023-05-24T18:49:42Z

bors try

bors · 2023-05-24T19:00:17Z

try

Build failed:

full-ci

Bromeon · 2023-05-24T19:25:01Z

@juliohq Unit test rect2_equiv_unary is failing, could you fix that?

If you rebase/merge latest master, the minimal CI (the checks you see at the bottom of this page) should be green.

juliohq · 2023-06-02T12:18:14Z

@juliohq Unit test rect2_equiv_unary is failing, could you fix that?

If you rebase/merge latest master, the minimal CI (the checks you see at the bottom of this page) should be green.

I'm sorry for the delay, I've been a bit busy these days. I'm going to rebase now!

Bromeon · 2023-06-06T09:02:27Z

@juliohq Unit test rect2_equiv_unary is failing, could you fix that?

Could you address that? 🙂

Also, please squash the commits in accordance with contribution guidelines.

Cankyre · 2023-06-13T15:24:56Z

itest/rust/src/rect2_test.rs

+};
+
+#[itest]
+fn rect2_equiv_unary() {


Hey! This test seems to fail due to floating-point arithmetic.
Idk if it should be prefered to solve floating-point or to use is_equal_approx.

(See: itests log)

That's the whole point why is_equal_approx exists 😉

Bromeon · 2023-06-17T15:10:26Z

Implemented changes myself as I didn't hear back in 2 weeks.

bors r+

242: Reimplement Rect2 functions r=Bromeon a=juliohq Reimplement Rect2 functions as proposed by #209. It's missing to reimplement `is_infinite()` somehow, since it needs an `inf` type. Co-authored-by: juliohq <[email protected]> Co-authored-by: Jan Haller <[email protected]>

bors · 2023-06-17T15:18:56Z

Build failed:

full-ci

Bromeon · 2023-06-17T15:22:26Z

Again network issue...

bors retry

bors · 2023-06-17T15:26:45Z

Already running a review

bors · 2023-06-17T15:35:04Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

lilizoey requested changes Apr 23, 2023

View reviewed changes

godot-core/src/builtin/rect2.rs Outdated Show resolved Hide resolved

godot-core/src/builtin/rect2.rs Outdated Show resolved Hide resolved

godot-core/src/builtin/rect2.rs Show resolved Hide resolved

godot-core/src/builtin/rect2.rs Show resolved Hide resolved

lilizoey added feature Adds functionality to the library c: core Core components labels Apr 23, 2023

Bromeon reviewed Apr 23, 2023

View reviewed changes

godot-core/src/builtin/rect2.rs Outdated Show resolved Hide resolved

godot-core/src/builtin/rect2.rs Outdated Show resolved Hide resolved

juliohq force-pushed the reimpl-rect2 branch from 64b0d9b to 12c8281 Compare April 24, 2023 11:42

juliohq requested review from lilizoey and Bromeon April 24, 2023 11:44

Bromeon reviewed Apr 24, 2023

View reviewed changes

juliohq requested a review from Bromeon April 24, 2023 17:14

Bromeon reviewed May 8, 2023

View reviewed changes

lilizoey mentioned this pull request May 15, 2023

Reimplement Rect2, Rect2i, Aabb, and Plane in rust #209

Closed

4 tasks

juliohq force-pushed the reimpl-rect2 branch from 7878392 to 2084747 Compare May 19, 2023 16:46

bors bot added a commit that referenced this pull request May 24, 2023

Try #242:

df55343

juliohq force-pushed the reimpl-rect2 branch from 2084747 to 247f045 Compare June 2, 2023 12:18

Bromeon linked an issue Jun 13, 2023 that may be closed by this pull request

Reimplement Rect2, Rect2i, Aabb, and Plane in rust #209

Closed

4 tasks

Cankyre reviewed Jun 13, 2023

View reviewed changes

lilizoey mentioned this pull request Jun 14, 2023

Tracking issue for builtin feature-completeness #310

Open

33 tasks

Reimplement Rect2 functions

2eeff84

Bromeon force-pushed the reimpl-rect2 branch from 247f045 to 0a53448 Compare June 17, 2023 15:04

Follow-up fixes

df42396

Bromeon force-pushed the reimpl-rect2 branch from 0a53448 to df42396 Compare June 17, 2023 15:09

bors bot merged commit 08ea421 into godot-rust:master Jun 17, 2023

juliohq deleted the reimpl-rect2 branch June 17, 2023 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement Rect2 functions #242

Reimplement Rect2 functions #242

juliohq commented Apr 23, 2023 •

edited

Loading

lilizoey left a comment •

edited

Loading

Bromeon left a comment •

edited

Loading

Bromeon Apr 24, 2023

juliohq Apr 24, 2023

Bromeon Apr 24, 2023

Bromeon commented Apr 24, 2023

juliohq commented Apr 26, 2023 •

edited

Loading

lilizoey commented Apr 26, 2023

juliohq commented Apr 26, 2023

lilizoey commented Apr 26, 2023

Bromeon commented Apr 26, 2023

Bromeon left a comment

Bromeon May 8, 2023

juliohq May 9, 2023

Bromeon May 9, 2023

Bromeon May 14, 2023

juliohq May 14, 2023

Bromeon May 14, 2023

juliohq May 14, 2023

Bromeon May 14, 2023

GodotRust commented May 19, 2023

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

Bromeon commented May 24, 2023

juliohq commented Jun 2, 2023

Bromeon commented Jun 6, 2023

Cankyre Jun 13, 2023 •

edited

Loading

Bromeon Jun 13, 2023

Bromeon commented Jun 17, 2023

bors bot commented Jun 17, 2023

Bromeon commented Jun 17, 2023

bors bot commented Jun 17, 2023

bors bot commented Jun 17, 2023

	/// Asserts that two values are approximately equal, using the provided `func`
	/// for equality checking.
	#[macro_export]
	macro_rules! assert_eq_approx {
	($a:expr, $b:expr, $func:expr $(,)?) => {
	match ($a, $b) {
	(a, b) => {
	assert!(($func)(a,b), "\n left: {:?},\n right: {:?}", $a, $b);
	}
	}
	};
	($a:expr, $b:expr, $func:expr, $($t:tt)+) => {
	match ($a, $b) {
	(a, b) => {
	assert!(($func)(a,b), "\n left: {:?},\n right: {:?},\n{}", $a, $b, format_args!($($t)+));
	}
	}
	};
	}

Reimplement Rect2 functions #242

Reimplement Rect2 functions #242

Conversation

juliohq commented Apr 23, 2023 • edited Loading

lilizoey left a comment • edited Loading

Choose a reason for hiding this comment

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Apr 24, 2023

juliohq commented Apr 26, 2023 • edited Loading

lilizoey commented Apr 26, 2023

juliohq commented Apr 26, 2023

lilizoey commented Apr 26, 2023

Bromeon commented Apr 26, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GodotRust commented May 19, 2023

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

try

Bromeon commented May 24, 2023

juliohq commented Jun 2, 2023

Bromeon commented Jun 6, 2023

Cankyre Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Jun 17, 2023

bors bot commented Jun 17, 2023

Bromeon commented Jun 17, 2023

bors bot commented Jun 17, 2023

bors bot commented Jun 17, 2023

juliohq commented Apr 23, 2023 •

edited

Loading

lilizoey left a comment •

edited

Loading

Bromeon left a comment •

edited

Loading

juliohq commented Apr 26, 2023 •

edited

Loading

Cankyre Jun 13, 2023 •

edited

Loading