Merge pull request #1300 from Yarwin/support-negative-indices-in-array-slices

Add `SignedRange` for negative indices in range ops

Author: Bromeon

godot-core/src/builtin/collections/array.rs

@@ -15,6 +15,7 @@ use sys::{ffi_methods, interface_fn, GodotFfi};
 use crate::builtin::*;
 use crate::meta;
 use crate::meta::error::{ConvertError, FromGodotError, FromVariantError};
+use crate::meta::signed_range::SignedRange;
 use crate::meta::{
     element_godot_type_name, element_variant_type, ArrayElement, AsArg, ClassName, ElementType,
     ExtVariantType, FromGodot, GodotConvert, GodotFfiVariant, GodotType, PropertyHintInfo, RefArg,
@@ -108,6 +109,35 @@ use crate::registry::property::{BuiltinExport, Export, Var};
 /// // ...and so on.
 /// ```
 ///
+/// # Working with signed ranges and steps
+///
+/// For negative indices, use [`wrapped()`](crate::meta::wrapped).
+///
+/// ```no_run
+/// # use godot::builtin::array;
+/// # use godot::meta::wrapped;
+/// let arr = array![0, 1, 2, 3, 4, 5];
+///
+/// // The values of `begin` (inclusive) and `end` (exclusive) will be clamped to the array size.
+/// let clamped_array = arr.subarray_deep(999..99999, None);
+/// assert_eq!(clamped_array, array![]);
+///
+/// // If either `begin` or `end` is negative, its value is relative to the end of the array.
+/// let sub = arr.subarray_shallow(wrapped(-1..-5), None);
+/// assert_eq!(sub, array![5, 3]);
+///
+/// // If `end` is not specified, the range spans through whole array.
+/// let sub = arr.subarray_deep(1.., None);
+/// assert_eq!(sub, array![1, 2, 3, 4, 5]);
+/// let other_clamped_array = arr.subarray_shallow(5.., Some(2));
+/// assert_eq!(other_clamped_array, array![5]);
+///
+/// // If specified, `step` is the relative index between source elements. It can be negative,
+/// // in which case `begin` must be higher than `end`.
+/// let sub = arr.subarray_shallow(wrapped(-1..-5), Some(-2));
+/// assert_eq!(sub, array![5, 3]);
+/// ```
+///
 /// # Thread safety
 ///
 /// Usage is safe if the `Array` is used on a single thread only. Concurrent reads on
@@ -526,57 +556,41 @@ impl<T: ArrayElement> Array<T> {
         result.with_cache(self)
     }
 
-    /// Returns a sub-range `begin..end`, as a new array.
-    ///
-    /// The values of `begin` (inclusive) and `end` (exclusive) will be clamped to the array size.
-    ///
-    /// If specified, `step` is the relative index between source elements. It can be negative,
-    /// in which case `begin` must be higher than `end`. For example,
-    /// `Array::from(&[0, 1, 2, 3, 4, 5]).slice(5, 1, -2)` returns `[5, 3]`.
+    /// Returns a sub-range `begin..end` as a new `Array`.
     ///
     /// Array elements are copied to the slice, but any reference types (such as `Array`,
     /// `Dictionary` and `Object`) will still refer to the same value. To create a deep copy, use
     /// [`subarray_deep()`][Self::subarray_deep] instead.
     ///
     /// _Godot equivalent: `slice`_
     #[doc(alias = "slice")]
-    // TODO(v0.3): change to i32 like NodePath::slice/subpath() and support+test negative indices.
-    pub fn subarray_shallow(&self, begin: usize, end: usize, step: Option<isize>) -> Self {
-        self.subarray_impl(begin, end, step, false)
+    pub fn subarray_shallow(&self, range: impl SignedRange, step: Option<i32>) -> Self {
+        self.subarray_impl(range, step, false)
     }
 
-    /// Returns a sub-range `begin..end`, as a new `Array`.
-    ///
-    /// The values of `begin` (inclusive) and `end` (exclusive) will be clamped to the array size.
-    ///
-    /// If specified, `step` is the relative index between source elements. It can be negative,
-    /// in which case `begin` must be higher than `end`. For example,
-    /// `Array::from(&[0, 1, 2, 3, 4, 5]).slice(5, 1, -2)` returns `[5, 3]`.
+    /// Returns a sub-range `begin..end` as a new `Array`.
     ///
     /// All nested arrays and dictionaries are duplicated and will not be shared with the original
     /// array. Note that any `Object`-derived elements will still be shallow copied. To create a
     /// shallow copy, use [`subarray_shallow()`][Self::subarray_shallow] instead.
     ///
     /// _Godot equivalent: `slice`_
     #[doc(alias = "slice")]
-    // TODO(v0.3): change to i32 like NodePath::slice/subpath() and support+test negative indices.
-    pub fn subarray_deep(&self, begin: usize, end: usize, step: Option<isize>) -> Self {
-        self.subarray_impl(begin, end, step, true)
+    pub fn subarray_deep(&self, range: impl SignedRange, step: Option<i32>) -> Self {
+        self.subarray_impl(range, step, true)
     }
 
-    fn subarray_impl(&self, begin: usize, end: usize, step: Option<isize>, deep: bool) -> Self {
+    // Note: Godot will clamp values by itself.
+    fn subarray_impl(&self, range: impl SignedRange, step: Option<i32>, deep: bool) -> Self {
         assert_ne!(step, Some(0), "subarray: step cannot be zero");
 
-        let len = self.len();
-        let begin = begin.min(len);
-        let end = end.min(len);
         let step = step.unwrap_or(1);
+        let (begin, end) = range.signed();
+        let end = end.unwrap_or(i32::MAX as i64);
 
         // SAFETY: The type of the array is `T` and we convert the returned array to an `Array<T>` immediately.
-        let subarray: VariantArray = unsafe {
-            self.as_inner()
-                .slice(to_i64(begin), to_i64(end), step.try_into().unwrap(), deep)
-        };
+        let subarray: VariantArray =
+            unsafe { self.as_inner().slice(begin, end, step as i64, deep) };
 
         // SAFETY: slice() returns a typed array with the same type as Self.
         let result = unsafe { subarray.assume_type() };

godot-core/src/builtin/collections/packed_array.rs

@@ -19,6 +19,7 @@ use crate::builtin::collections::extend_buffer::ExtendBufferTrait;
 use crate::builtin::*;
 use crate::classes::file_access::CompressionMode;
 use crate::meta;
+use crate::meta::signed_range::SignedRange;
 use crate::meta::{AsArg, FromGodot, GodotConvert, PackedArrayElement, ToGodot};
 use crate::obj::EngineEnum;
 use crate::registry::property::{Export, Var};
@@ -226,18 +227,31 @@ impl<T: PackedArrayElement> PackedArray<T> {
 
     /// Returns a sub-range `begin..end`, as a new packed array.
     ///
-    /// This method is called `slice()` in Godot.
     /// The values of `begin` (inclusive) and `end` (exclusive) will be clamped to the array size.
-    ///
     /// To obtain Rust slices, see [`as_slice`][Self::as_slice] and [`as_mut_slice`][Self::as_mut_slice].
+    ///
+    /// # Usage
+    /// For negative indices, use [`wrapped()`](crate::meta::wrapped).
+    ///
+    /// ```no_run
+    /// # use godot::builtin::PackedArray;
+    /// # use godot::meta::wrapped;
+    /// let array = PackedArray::from([10, 20, 30, 40, 50]);
+    ///
+    /// // If either `begin` or `end` is negative, its value is relative to the end of the array.
+    /// let sub = array.subarray(wrapped(-4..-2));
+    /// assert_eq!(sub, PackedArray::from([20, 30]));
+    ///
+    /// // If `end` is not specified, the resulting subarray will span to the end of the array.
+    /// let sub = array.subarray(2..);
+    /// assert_eq!(sub, PackedArray::from([30, 40, 50]));
+    /// ```
+    ///
+    /// _Godot equivalent: `slice`_
     #[doc(alias = "slice")]
-    // TODO(v0.3): change to i32 like NodePath::slice/subpath() and support+test negative indices.
-    pub fn subarray(&self, begin: usize, end: usize) -> Self {
-        let len = self.len();
-        let begin = begin.min(len);
-        let end = end.min(len);
-
-        T::op_slice(self.as_inner(), to_i64(begin), to_i64(end))
+    // Note: Godot will clamp values by itself.
+    pub fn subarray(&self, range: impl SignedRange) -> Self {
+        T::op_slice(self.as_inner(), range)
     }
 
     /// Returns a shared Rust slice of the array.

godot-core/src/builtin/collections/packed_array_element.rs

@@ -9,6 +9,7 @@ use sys::{interface_fn, GodotFfi, SysPtr};
 
 use crate::builtin::collections::extend_buffer::{ExtendBuffer, ExtendBufferTrait};
 use crate::builtin::PackedArray;
+use crate::meta::signed_range::SignedRange;
 use crate::meta::{CowArg, FromGodot, GodotType, ToGodot};
 use crate::registry::property::builtin_type_string;
 use crate::{builtin, sys};
@@ -126,7 +127,7 @@ pub trait PackedArrayElement: GodotType + Clone + ToGodot + FromGodot {
     fn op_append_array(inner: Self::Inner<'_>, other: &PackedArray<Self>);
 
     #[doc(hidden)]
-    fn op_slice(inner: Self::Inner<'_>, begin: i64, end: i64) -> PackedArray<Self>;
+    fn op_slice(inner: Self::Inner<'_>, range: impl SignedRange) -> PackedArray<Self>;
 
     #[doc(hidden)]
     fn op_find(inner: Self::Inner<'_>, value: CowArg<'_, Self>, from: i64) -> i64;
@@ -285,8 +286,9 @@ macro_rules! impl_packed_array_element {
                 inner.append_array(other);
             }
 
-            fn op_slice(inner: Self::Inner<'_>, begin: i64, end: i64) -> PackedArray<Self> {
-                inner.slice(begin, end)
+            fn op_slice(inner: Self::Inner<'_>, range: impl $crate::meta::signed_range::SignedRange) -> PackedArray<Self> {
+                let (begin, end) = range.signed();
+                inner.slice(begin, end.unwrap_or(i32::MAX as i64))
             }
 
             fn op_find(inner: Self::Inner<'_>, value: CowArg<'_, Self>, from: i64) -> i64 {

godot-core/src/builtin/mod.rs

@@ -113,6 +113,9 @@ pub mod inner {
     pub use crate::gen::builtin_classes::*;
 }
 
+// ----------------------------------------------------------------------------------------------------------------------------------------------
+// Conversion functions
+
 pub(crate) fn to_i64(i: usize) -> i64 {
     i.try_into().unwrap()
 }

godot-core/src/builtin/string/mod.rs

@@ -13,8 +13,6 @@ mod node_path;
 mod string_macros;
 mod string_name;
 
-use std::ops;
-
 pub use gstring::*;
 pub use node_path::NodePath;
 pub use string_name::*;
@@ -70,71 +68,6 @@ pub enum Encoding {
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
-/// Returns a tuple of `(from, len)` from a Rust range.
-///
-/// Unbounded upper bounds are represented by `len = -1`.
-fn to_godot_fromlen_neg1<R>(range: R) -> (i64, i64)
-where
-    R: ops::RangeBounds<usize>,
-{
-    let from = match range.start_bound() {
-        ops::Bound::Included(&n) => n as i64,
-        ops::Bound::Excluded(&n) => (n as i64) + 1,
-        ops::Bound::Unbounded => 0,
-    };
-
-    let len = match range.end_bound() {
-        ops::Bound::Included(&n) => {
-            let to = (n + 1) as i64;
-            debug_assert!(
-                from <= to,
-                "range: start ({from}) > inclusive end ({n}) + 1"
-            );
-            to - from
-        }
-        ops::Bound::Excluded(&n) => {
-            let to = n as i64;
-            debug_assert!(from <= to, "range: start ({from}) > exclusive end ({to})");
-            to - from
-        }
-        ops::Bound::Unbounded => -1,
-    };
-
-    (from, len)
-}
-
-/// Returns a tuple of `(from, len)` from a Rust range.
-///
-/// Unbounded upper bounds are represented by `i32::MAX` (yes, not `i64::MAX` -- since Godot treats some indexes as 32-bit despite being
-/// declared `i64` in GDExtension API).
-fn to_godot_fromlen_i32max<R>(range: R) -> (i64, i64)
-where
-    R: ops::RangeBounds<usize>,
-{
-    let (from, len) = to_godot_fromlen_neg1(range);
-    if len == -1 {
-        // Use i32 here because Godot may wrap around larger values (see Rustdoc).
-        (from, i32::MAX as i64)
-    } else {
-        (from, len)
-    }
-}
-
-/// Returns a tuple of `(from, to)` from a Rust range.
-///
-/// Unbounded upper bounds are represented by `to = 0`.
-fn to_godot_fromto<R>(range: R) -> (i64, i64)
-where
-    R: ops::RangeBounds<usize>,
-{
-    let (from, len) = to_godot_fromlen_neg1(range);
-    if len == -1 {
-        (from, 0)
-    } else {
-        (from, from + len)
-    }
-}
-
 fn populated_or_none(s: GString) -> Option<GString> {
     if s.is_empty() {
         None

godot-core/src/builtin/string/node_path.rs

@@ -12,6 +12,7 @@ use godot_ffi::{ffi_methods, ExtVariantType, GdextBuild, GodotFfi};
 
 use super::{GString, StringName};
 use crate::builtin::inner;
+use crate::meta::signed_range::SignedRange;
 
 /// A pre-parsed scene tree path.
 ///
@@ -127,29 +128,46 @@ impl NodePath {
     /// Returns the range `begin..exclusive_end` as a new `NodePath`.
     ///
     /// The absolute value of `begin` and `exclusive_end` will be clamped to [`get_total_count()`][Self::get_total_count].
-    /// So, to express "until the end", you can simply pass a large value for `exclusive_end`, such as `i32::MAX`.
     ///
-    /// If either `begin` or `exclusive_end` are negative, they will be relative to the end of the `NodePath`.  \
-    /// For example, `path.subpath(0, -2)` is a shorthand for `path.subpath(0, path.get_total_count() - 2)`.
+    /// # Usage
+    /// For negative indices, use [`wrapped()`](crate::meta::wrapped).
+    ///
+    /// ```no_run
+    /// # use godot::builtin::NodePath;
+    /// # use godot::meta::wrapped;
+    /// let path = NodePath::from("path/to/Node:with:props");
+    ///
+    /// // If upper bound is not defined, `exclusive_end` will span to the end of the `NodePath`.
+    /// let sub = path.subpath(2..);
+    /// assert_eq!(sub, ":props".into());
+    ///
+    /// // If either `begin` or `exclusive_end` are negative, they will be relative to the end of the `NodePath`.
+    /// let total_count = path.get_total_count();
+    /// let wrapped_sub = path.subpath(wrapped(0..-2));
+    /// let normal_sub = path.subpath(0..total_count - 2);
+    /// // Both are equal to "path/to/Node".
+    /// assert_eq!(wrapped_sub, normal_sub);
+    /// ```
     ///
     /// _Godot equivalent: `slice`_
     ///
     /// # Compatibility
-    /// The `slice()` behavior for Godot <= 4.3 is unintuitive, see [#100954](https://github.com/godotengine/godot/pull/100954). godot-rust
+    /// The `slice()` behavior for Godot <= 4.3 is unintuitive, see [#100954](https://github.com/godotengine/godot/pull/100954). Godot-rust
     /// automatically changes this to the fixed version for Godot 4.4+, even when used in older versions. So, the behavior is always the same.
     // i32 used because it can be negative and many Godot APIs use this, see https://github.com/godot-rust/gdext/pull/982/files#r1893732978.
     #[cfg(since_api = "4.3")]
     #[doc(alias = "slice")]
-    pub fn subpath(&self, begin: i32, exclusive_end: i32) -> NodePath {
+    pub fn subpath(&self, range: impl SignedRange) -> NodePath {
+        let (from, exclusive_end) = range.signed();
         // Polyfill for bug https://github.com/godotengine/godot/pull/100954, fixed in 4.4.
         let begin = if GdextBuild::since_api("4.4") {
-            begin
+            from
         } else {
-            let name_count = self.get_name_count() as i32;
-            let subname_count = self.get_subname_count() as i32;
+            let name_count = self.get_name_count() as i64;
+            let subname_count = self.get_subname_count() as i64;
             let total_count = name_count + subname_count;
 
-            let mut begin = begin.clamp(-total_count, total_count);
+            let mut begin = from.clamp(-total_count, total_count);
             if begin < 0 {
                 begin += total_count;
             }
@@ -159,7 +177,8 @@ impl NodePath {
             begin
         };
 
-        self.as_inner().slice(begin as i64, exclusive_end as i64)
+        self.as_inner()
+            .slice(begin, exclusive_end.unwrap_or(i32::MAX as i64))
     }
 
     crate::meta::declare_arg_method! {

godot-core/src/builtin/string/string_macros.rs

@@ -81,13 +81,13 @@ macro_rules! impl_shared_string_api {
 
             /// Count how many times `what` appears within `range`. Use `..` for full string search.
             pub fn count(&self, what: impl AsArg<GString>, range: impl std::ops::RangeBounds<usize>) -> usize {
-                let (from, to) = super::to_godot_fromto(range);
+                let (from, to) = $crate::meta::signed_range::to_godot_range_fromto(range);
                 self.as_inner().count(what, from, to) as usize
             }
 
             /// Count how many times `what` appears within `range`, case-insensitively. Use `..` for full string search.
             pub fn countn(&self, what: impl AsArg<GString>, range: impl std::ops::RangeBounds<usize>) -> usize {
-                let (from, to) = super::to_godot_fromto(range);
+                let (from, to) = $crate::meta::signed_range::to_godot_range_fromto(range);
                 self.as_inner().countn(what, from, to) as usize
             }
 
@@ -121,7 +121,7 @@ macro_rules! impl_shared_string_api {
             /// Returns a substring of this, as another `GString`.
             // TODO is there no efficient way to implement this for StringName by interning?
             pub fn substr(&self, range: impl std::ops::RangeBounds<usize>) -> GString {
-                let (from, len) = super::to_godot_fromlen_neg1(range);
+                let (from, len) = $crate::meta::signed_range::to_godot_range_fromlen(range, -1);
 
                 self.as_inner().substr(from, len)
             }
@@ -163,7 +163,7 @@ macro_rules! impl_shared_string_api {
 
             /// Returns a copy of the string without the specified index range.
             pub fn erase(&self, range: impl std::ops::RangeBounds<usize>) -> GString {
-                let (from, len) = super::to_godot_fromlen_i32max(range);
+                let (from, len) = $crate::meta::signed_range::to_godot_range_fromlen(range, i32::MAX as i64);
                 self.as_inner().erase(from, len)
             }