Reimplement Rect2 functions by juliohq · Pull Request #242 · godot-rust/gdext (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation45 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

juliohq

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

lilizoey

Member

@lilizoey lilizoey left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Bromeon

Bromeon

Member

@Bromeon Bromeon left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing:

I'd keep it simple, something like:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@Bromeon

@juliohq

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

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

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines 76 to 84

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),
);
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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,b:expr, b:expr,func:expr $(,)?) => {
match ($a, $b) {
(a, b) => {
assert!(($func)(a,b), "\n left: {:?},\n right: {:?}", a,a, a,b);
}
}
};
($a:expr, b:expr,b:expr, b:expr,func:expr, (((t:tt)+) => {
match ($a, $b) {
(a, b) => {
assert!(($func)(a,b), "\n left: {:?},\n right: {:?},\n{}", a,a, a,b, format_args!($($t)+));
}
}
};
}

@GodotRust

@Bromeon

bors bot added a commit that referenced this pull request

May 24, 2023

@bors

@bors

@Bromeon

@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

@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

@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.

@Bromeon Bromeon linked an issue

Jun 13, 2023

that may beclosed by this pull request

4 tasks

Cankyre

};
#[itest]
fn rect2_equiv_unary() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the whole point why is_equal_approx exists 😉

@juliohq @Bromeon

@Bromeon

@Bromeon

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

bors r+

bors bot added a commit that referenced this pull request

Jun 17, 2023

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 juliohenrique31501234@gmail.com Co-authored-by: Jan Haller bromeon@gmail.com

@bors

@Bromeon

Again network issue...

bors retry

@bors

@bors

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.