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 snapped to integer vectors #768

Merged
merged 1 commit into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions godot-core/src/builtin/vectors/vector2i.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ impl_vector_fns!(Vector2i, glam::IVec2, i32, (x, y));
impl_vector2x_fns!(Vector2i, i32);

impl Vector2i {
impl_integer_vector_fns!(x, y);

/// Constructs a new `Vector2i` from a [`Vector2`]. The floating point coordinates will be truncated.
#[inline]
pub const fn from_vector2(v: Vector2) -> Self {
Expand Down
2 changes: 2 additions & 0 deletions godot-core/src/builtin/vectors/vector3i.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ impl_vector_fns!(Vector3i, glam::IVec3, i32, (x, y, z));
impl_vector3x_fns!(Vector3i, i32);

impl Vector3i {
impl_integer_vector_fns!(x, y, z);

/// Constructs a new `Vector3i` from a [`Vector3`]. The floating point coordinates will be truncated.
#[inline]
pub const fn from_vector3(v: Vector3) -> Self {
Expand Down
2 changes: 2 additions & 0 deletions godot-core/src/builtin/vectors/vector4i.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ impl_vector_fns!(Vector4i, glam::IVec4, i32, (x, y, z, w));
impl_vector4x_fns!(Vector4i, i32);

impl Vector4i {
impl_integer_vector_fns!(x, y, z, w);

/// Constructs a new `Vector4i` from a [`Vector4`]. The floating point coordinates will be
/// truncated.
#[inline]
Expand Down
55 changes: 54 additions & 1 deletion godot-core/src/builtin/vectors/vector_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,60 @@ macro_rules! impl_vector_fns {
}
}

pub(super) fn snap_one(mut value: i32, step: i32) -> i32 {
assert!(
value != i32::MIN || step != -1,
"snapped() called on vector component i32::MIN with step component -1"
);
Comment on lines +461 to +464
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.


if step != 0 {
// Can overflow if step / 2 + value is not in range of i32.
let a = (step / 2).checked_add(value).expect(
"snapped() overflowed, this happened because step / 2 + component is not in range of i32",
);

// Manually implement `a.div_floor(step)` since Rust's native method is still unstable, as of 1.79.0.

// Can overflow with a == i32::MIN and step == -1 when value == i32::MIN.
let mut d = a / step;
// Can't overflow because if a == i32::MIN and step == -1, value == -2147483647.5 which is impossible.
let r = a % step;
if (r > 0 && step < 0) || (r < 0 && step > 0) {
// Can't overflow because if d == i32::MIN than a == i32::MIN and step == 1 and value == -2147483648.5 which is impossible.
d -= 1;
}

value = step * d;
}

value
}

/// Implements functions that are present only on integer vectors.
macro_rules! impl_integer_vector_fns {
(
// Names of the components, for example `x, y`.
$($comp:ident),*
) => {
/// A new vector with each component snapped to the closest multiple of the corresponding
/// component in `step`.
///
/// # Panics
/// On under- or overflow:
/// - If any component of `self` is [`i32::MIN`] while the same component on `step` is `-1`.
/// - If any component of `self` plus half of the same component of `step` is not in range on [`i32`].
pub fn snapped(self, step: Self) -> Self {
use crate::builtin::vectors::vector_macros::snap_one;

Self::new(
$(
snap_one(self.$comp, step.$comp)
),*
)
}
};
}

/// Implements functions that are present only on floating-point vectors.
macro_rules! impl_float_vector_fns {
(
Expand Down Expand Up @@ -642,7 +696,6 @@ macro_rules! impl_float_vector_fns {

/// A new vector with each component snapped to the closest multiple of the corresponding
/// component in `step`.
// TODO: also implement for integer vectors
#[inline]
pub fn snapped(self, step: Self) -> Self {
Self::new(
Expand Down
18 changes: 10 additions & 8 deletions itest/rust/src/builtin_tests/geometry/vector_test/vector2i_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,13 @@ fn sign() {
assert_eq!(b.sign(), b.as_inner().sign());
}

// TODO: implement snapped for integer vectors
// #[itest]
// fn snapped() {
// let a = Vector2i::new(12, 34);
// let b = Vector2i::new(5, -5);

// assert_eq!(a.snapped(b), a.as_inner().snapped(b));
// }
#[itest]
fn snapped() {
let a = Vector2i::new(12, 34);
let b = Vector2i::new(5, -5);
let c = Vector2i::new(0, 0);
let d = Vector2i::new(3, 0);

assert_eq!(a.snapped(b), a.as_inner().snapped(b));
assert_eq!(c.snapped(d), c.as_inner().snapped(d));
}
18 changes: 10 additions & 8 deletions itest/rust/src/builtin_tests/geometry/vector_test/vector3i_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,13 @@ fn sign() {
assert_eq!(b.sign(), b.as_inner().sign());
}

// TODO: implement snapped for integer vectors
// #[itest]
// fn snapped() {
// let a = Vector3i::new(12, 34, 56);
// let b = Vector3i::new(5, -5, 6);

// assert_eq!(a.snapped(b), a.as_inner().snapped(b));
// }
#[itest]
fn snapped() {
let a = Vector3i::new(12, 34, -56);
let b = Vector3i::new(5, -5, 6);
let c = Vector3i::new(0, 3, 0);
let d = Vector3i::new(3, 0, 0);

assert_eq!(a.snapped(b), a.as_inner().snapped(b));
assert_eq!(c.snapped(d), c.as_inner().snapped(d));
}
18 changes: 10 additions & 8 deletions itest/rust/src/builtin_tests/geometry/vector_test/vector4i_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,13 @@ fn sign() {
assert_eq!(b.sign(), b.as_inner().sign());
}

// TODO: implement snapped for integer vectors
// #[itest]
// fn snapped() {
// let a = Vector4i::new(12, 34, 56, 78);
// let b = Vector4i::new(5, -5, 6, -6);

// assert_eq!(a.snapped(b), a.as_inner().snapped(b));
// }
#[itest]
fn snapped() {
let a = Vector4i::new(12, 34, 56, -78);
let b = Vector4i::new(5, -5, 6, 6);
let c = Vector4i::new(0, 3, 0, 0);
let d = Vector4i::new(3, 0, -3, 0);

assert_eq!(a.snapped(b), a.as_inner().snapped(b));
assert_eq!(c.snapped(d), c.as_inner().snapped(d));
}
Loading