Add `snapped` to integer vectors #768

joriskleiber · 2024-06-16T03:23:08Z

This adds snapped for integer vectors based on this code.

joriskleiber · 2024-06-16T23:02:54Z

godot-core/src/builtin/vectors/vector_macros.rs

+
+                    // manual implement `a.div_floor(step)` since Rust's native method is still unstable, as of 1.79.0
+                    let mut d = a / step;
+                    let r = a % step;


a % step won't panic because if a is ever equal to i32::MIN(-2147483648) and step is -1, value needs to be -2147483647.5 which is imposible.

if value is i32::MIN and step is -1 then a is i32::MIN since -1 / 2 = 0 for i32.

Bromeon

Thank you!

Could you also add tests for zero steps? (Either one component 0, or the whole step vector).

Bromeon · 2024-06-17T07:51:22Z

godot-core/src/builtin/vectors/vector_macros.rs

+        /// A new vector with each component snapped to the closest multiple of the corresponding
+        /// component in `step`.
+        pub fn snapped(self, step: Self) -> Self {
+            #[inline]


#[inline] is mostly needed for cross-crate inlining (because Rust simply can't do it without that attribute), but I wouldn't use it for private/crate-local functions. The compiler (or LLVM) should be able to judge whether to inline such functions itself.

Additionally, the snap_one method is now duplicated for all vectors. I think it would be better to declare it once manually outside the macro.

#[inline] is mostly needed for cross-crate inlining (because Rust simply can't do it without that attribute)

Not entirely true anymore, rust will automatically inline small functions as of 1.75 rust-lang/rust#116505

Yeah, this is however hardly useful in practice -- even if a function is "simple", there's a chance that refactoring makes it non-simple and causes pessimizations. As such, always writing #[inline] when inlining is allowed is still the better practice...

But it's a start, I hope this extended in the not too distant future 🙂

joriskleiber · 2024-06-23T17:43:38Z

I don't know if there is a better way to handle the possible division overflow, but I thinks its better to panic with a message then just letting it crash.

godot-core/src/builtin/vectors/vector_macros.rs

Bromeon · 2024-06-26T16:30:22Z

godot-core/src/builtin/vectors/vector_macros.rs

+    assert!(
+        value != i32::MIN || step != -1,
+        "snapped() called on vector component i32::MIN with step component -1"
+    );


Is only this a problem? step == -1 would work?

No, I just realized how many overflow errors this function has... I have no idea how to handle all these, maybe you could give some guidance what the best way forward would be?

Limit the input range to the common usage (and panic for others) would be a start. Upon user request, we can then expand in the future.

GodotRust · 2024-06-27T21:06:57Z

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

joriskleiber · 2024-06-28T00:02:48Z

So I did the math (hopefully correct) and came up with these, in my opinion reasonable, panic conditions. I hope I didn't miss any other panics or overflows.

Bromeon

Thanks a lot for approaching this so carefully! Turns out the seemingly simple snapped is very involved 😮

PR looks good, just some small doc nitpicks and it should be ready!

godot-core/src/builtin/vectors/vector_macros.rs

Bromeon · 2024-06-28T07:21:48Z

godot-core/src/builtin/vectors/vector_macros.rs

+    assert!(
+        value != i32::MIN || step >= 0,
+        "snapped() called on vector component i32::MIN with step component smaller then 0"
+    );
+    assert!(
+        value != i32::MAX || step <= 1,
+        "snapped() called on vector component i32::MAX with step component greater then 1"
+    );


Suggested change

assert!(

value != i32::MIN || step >= 0,

"snapped() called on vector component i32::MIN with step component smaller then 0"

);

assert!(

value != i32::MAX || step <= 1,

"snapped() called on vector component i32::MAX with step component greater then 1"

);

assert!(

value != i32::MIN || step >= 0,

"snapped() called on vector component i32::MIN with step component less than 0"

);

assert!(

value != i32::MAX || step <= 1,

"snapped() called on vector component i32::MAX with step component greater than 1"

);

Bromeon · 2024-06-28T07:24:21Z

godot-core/src/builtin/vectors/vector_macros.rs

+        /// # Panics
+        /// If any component of `self` is `i32::MIN` while the same component on `step` smaller then `0` or
+        /// if any component of `self` is `i32::MAX` while the same component on `step` greater then `1`.


Maybe this could be worded in a way makes clear that panics only happen in under/overflow scenarios, not general usage and especially not 0 steps:

Suggested change

/// # Panics

/// If any component of `self` is `i32::MIN` while the same component on `step` smaller then `0` or

/// if any component of `self` is `i32::MAX` while the same component on `step` greater then `1`.

/// # Panics

/// On under- or overflow:

/// - If any component of `self` is `i32::MIN` while the same component on `step` less than `0`.

/// - If any component of `self` is `i32::MAX` while the same component on `step` greater than `1`.

joriskleiber · 2024-07-01T13:25:35Z

As expected there where more possible overflow scenarios. For whatever reason i thought addition could only overflow for i32::MIN - 1 or i32::MAX + 1.

The description for the second panic condition is horrific but I don't know how to word it otherwise.

Bromeon

Thanks for working out the conditions so carefully!

I think we might need to keep an eye on the performance; with all the overflow checks, it could happen that the happy path is now unnecessarily slow. But if this becomes a problem, we can probably use debug_assert! or similar.

Ughuuu · 2024-07-08T18:16:53Z

I second them becoming debug_asserts, but probably would need to be a bigger change over more things.

Bromeon · 2024-07-09T18:07:58Z

Thanks!

Bromeon added feature Adds functionality to the library c: core Core components labels Jun 16, 2024

joriskleiber marked this pull request as draft June 16, 2024 22:35

joriskleiber commented Jun 16, 2024

View reviewed changes

Bromeon reviewed Jun 17, 2024

View reviewed changes

joriskleiber force-pushed the snapped-int-vectors branch from 7da3b4d to d444540 Compare June 22, 2024 13:09

joriskleiber marked this pull request as ready for review June 23, 2024 01:29

joriskleiber force-pushed the snapped-int-vectors branch from d444540 to 8e5e481 Compare June 26, 2024 13:25

Bromeon reviewed Jun 26, 2024

View reviewed changes

joriskleiber force-pushed the snapped-int-vectors branch from 8e5e481 to c0449ff Compare June 27, 2024 20:58

joriskleiber force-pushed the snapped-int-vectors branch from c0449ff to 56b9e74 Compare June 27, 2024 23:58

Bromeon reviewed Jun 28, 2024

View reviewed changes

joriskleiber force-pushed the snapped-int-vectors branch from 56b9e74 to 12ffa74 Compare July 1, 2024 13:18

Add snapped to integer vectors

26c5e10

joriskleiber force-pushed the snapped-int-vectors branch from 12ffa74 to 26c5e10 Compare July 1, 2024 13:20

Bromeon approved these changes Jul 8, 2024

View reviewed changes

Bromeon added this pull request to the merge queue Jul 8, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 8, 2024

Bromeon added this pull request to the merge queue Jul 8, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 8, 2024

Bromeon added this pull request to the merge queue Jul 9, 2024

Merged via the queue into godot-rust:master with commit 7641696 Jul 9, 2024

Uh oh!

Add snapped to integer vectors #768

Add snapped to integer vectors #768

Uh oh!

Conversation

joriskleiber commented Jun 16, 2024

Uh oh!

joriskleiber Jun 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Bromeon Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joriskleiber commented Jun 23, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GodotRust commented Jun 27, 2024

Uh oh!

joriskleiber commented Jun 28, 2024

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joriskleiber commented Jul 1, 2024

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ughuuu commented Jul 8, 2024

Uh oh!

Uh oh!

Bromeon commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Add `snapped` to integer vectors #768

Add `snapped` to integer vectors #768

joriskleiber Jun 16, 2024 •

edited

Loading

Bromeon Jun 17, 2024 •

edited

Loading