From 21b89063b510882e3ea0a5ba34ec3129c85177e7 Mon Sep 17 00:00:00 2001 From: Yarvin Date: Fri, 5 Sep 2025 14:38:46 +0200 Subject: [PATCH 1/2] Implement trait GodotRange, used to convert Rust ranges to values expected by Godot. Support negative indices for Array. Fix Array docs. --- godot-core/src/builtin/collections/array.rs | 83 +++++++++++++----- .../src/builtin/collections/packed_array.rs | 32 +++++-- .../collections/packed_array_element.rs | 9 +- godot-core/src/builtin/mod.rs | 3 + godot-core/src/builtin/string/mod.rs | 67 --------------- godot-core/src/builtin/string/node_path.rs | 38 ++++++--- .../src/builtin/string/string_macros.rs | 15 +++- godot-core/src/meta/godot_range.rs | 84 +++++++++++++++++++ godot-core/src/meta/mod.rs | 1 + godot-core/src/tools/gfile.rs | 2 +- .../builtin_tests/containers/array_test.rs | 36 +++++++- .../containers/packed_array_test.rs | 12 ++- .../builtin_tests/string/node_path_test.rs | 25 +++--- 13 files changed, 275 insertions(+), 132 deletions(-) create mode 100644 godot-core/src/meta/godot_range.rs diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index 4089dd8e9..f74e1377b 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -7,6 +7,7 @@ use std::cell::OnceCell; use std::marker::PhantomData; +use std::ops::RangeBounds; use std::{cmp, fmt}; use godot_ffi as sys; @@ -15,6 +16,7 @@ use sys::{ffi_methods, interface_fn, GodotFfi}; use crate::builtin::*; use crate::meta; use crate::meta::error::{ConvertError, FromGodotError, FromVariantError}; +use crate::meta::godot_range::GodotRange; use crate::meta::{ element_godot_type_name, element_variant_type, ArrayElement, AsArg, ClassName, ElementType, ExtVariantType, FromGodot, GodotConvert, GodotFfiVariant, GodotType, PropertyHintInfo, RefArg, @@ -526,13 +528,34 @@ impl Array { result.with_cache(self) } - /// Returns a sub-range `begin..end`, as a new array. + /// 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 either `begin` or `end` are negative, their value is relative to the end of the array. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::array; + /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_shallow(-1..-5, None), array![5, 3]); + /// ``` + /// + /// If `end` is not specified, the range spans through whole array. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::array; + /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_shallow(1.., None), array![1, 2, 3, 4, 5]); + /// ``` + /// /// 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]`. + /// in which case `begin` must be higher than `end`. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::array; + /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_shallow(-1..-5, Some(-2)), array![5, 3]); + /// ``` /// /// 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 @@ -540,18 +563,38 @@ impl Array { /// /// _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) -> Self { - self.subarray_impl(begin, end, step, false) + pub fn subarray_shallow(&self, range: impl RangeBounds, step: Option) -> Self { + self.subarray_impl(range, step, false) } - /// Returns a sub-range `begin..end`, as a new `Array`. + /// 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 either `begin` or `end` are negative, their value is relative to the end of the array. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::array; + /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_deep(-1..-5, None), array![5, 3]); + /// ``` + /// + /// If `end` is not specified, the range spans through whole array. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::array; + /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_deep(1.., None), array![1, 2, 3, 4, 5]); + /// ``` + /// /// 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]`. + /// in which case `begin` must be higher than `end`. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::array; + /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_deep(-1..-5, Some(-2)), array![5, 3]); + /// ``` /// /// 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 @@ -559,24 +602,24 @@ impl Array { /// /// _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) -> Self { - self.subarray_impl(begin, end, step, true) + pub fn subarray_deep(&self, range: impl RangeBounds, step: Option) -> Self { + self.subarray_impl(range, step, true) } - fn subarray_impl(&self, begin: usize, end: usize, step: Option, deep: bool) -> Self { + // Note: Godot will clamp values by itself. + fn subarray_impl(&self, range: impl GodotRange, step: Option, 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.to_godot_range_fromto(); + + // Unbounded upper bounds are represented by `i32::MAX` instead of `i64::MAX`, + // since Godot treats some indexes as 32-bit despite being declared `i64` in GDExtension API. + 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` 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() }; diff --git a/godot-core/src/builtin/collections/packed_array.rs b/godot-core/src/builtin/collections/packed_array.rs index d0c6a1317..432eef1a5 100644 --- a/godot-core/src/builtin/collections/packed_array.rs +++ b/godot-core/src/builtin/collections/packed_array.rs @@ -10,6 +10,7 @@ #![allow(clippy::result_unit_err)] use std::iter::FromIterator; +use std::ops::RangeBounds; use std::{fmt, ops, ptr}; use godot_ffi as sys; @@ -226,18 +227,33 @@ impl PackedArray { /// 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. /// + /// If either `begin` or `end` are negative, their value is relative to the end of the array. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::PackedArray; + /// let array = PackedArray::from([10, 20, 30, 40, 50]); + /// assert_eq!(array.subarray(-4..-2), PackedArray::from([20, 30])); + /// ``` + /// + /// If `end` is not specified, the resulting subarray will span to the end of the array. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::PackedArray; + /// let array = PackedArray::from([10, 20, 30, 40, 50]); + /// assert_eq!(array.subarray(2..), PackedArray::from([30, 40, 50])); + /// ``` + /// /// To obtain Rust slices, see [`as_slice`][Self::as_slice] and [`as_mut_slice`][Self::as_mut_slice]. + /// + /// _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 RangeBounds) -> Self { + T::op_slice(self.as_inner(), range) } /// Returns a shared Rust slice of the array. diff --git a/godot-core/src/builtin/collections/packed_array_element.rs b/godot-core/src/builtin/collections/packed_array_element.rs index 8f30bfdec..246461892 100644 --- a/godot-core/src/builtin/collections/packed_array_element.rs +++ b/godot-core/src/builtin/collections/packed_array_element.rs @@ -4,11 +4,13 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +use std::ops::RangeBounds; use sys::{interface_fn, GodotFfi, SysPtr}; use crate::builtin::collections::extend_buffer::{ExtendBuffer, ExtendBufferTrait}; use crate::builtin::PackedArray; +use crate::meta::godot_range::GodotRange; use crate::meta::{CowArg, FromGodot, GodotType, ToGodot}; use crate::registry::property::builtin_type_string; use crate::{builtin, sys}; @@ -126,7 +128,7 @@ pub trait PackedArrayElement: GodotType + Clone + ToGodot + FromGodot { fn op_append_array(inner: Self::Inner<'_>, other: &PackedArray); #[doc(hidden)] - fn op_slice(inner: Self::Inner<'_>, begin: i64, end: i64) -> PackedArray; + fn op_slice(inner: Self::Inner<'_>, range: impl RangeBounds) -> PackedArray; #[doc(hidden)] fn op_find(inner: Self::Inner<'_>, value: CowArg<'_, Self>, from: i64) -> i64; @@ -285,8 +287,9 @@ macro_rules! impl_packed_array_element { inner.append_array(other); } - fn op_slice(inner: Self::Inner<'_>, begin: i64, end: i64) -> PackedArray { - inner.slice(begin, end) + fn op_slice(inner: Self::Inner<'_>, range: impl RangeBounds) -> PackedArray { + let (begin, end) = range.to_godot_range_fromto(); + inner.slice(begin, end.unwrap_or(i32::MAX as i64)) } fn op_find(inner: Self::Inner<'_>, value: CowArg<'_, Self>, from: i64) -> i64 { diff --git a/godot-core/src/builtin/mod.rs b/godot-core/src/builtin/mod.rs index 1a00cdf55..eaa5a9308 100644 --- a/godot-core/src/builtin/mod.rs +++ b/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() } diff --git a/godot-core/src/builtin/string/mod.rs b/godot-core/src/builtin/string/mod.rs index 0c29283bd..aaab61279 100644 --- a/godot-core/src/builtin/string/mod.rs +++ b/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::*; @@ -69,71 +67,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(range: R) -> (i64, i64) -where - R: ops::RangeBounds, -{ - 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(range: R) -> (i64, i64) -where - R: ops::RangeBounds, -{ - 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(range: R) -> (i64, i64) -where - R: ops::RangeBounds, -{ - let (from, len) = to_godot_fromlen_neg1(range); - if len == -1 { - (from, 0) - } else { - (from, from + len) - } -} - fn populated_or_none(s: GString) -> Option { if s.is_empty() { None diff --git a/godot-core/src/builtin/string/node_path.rs b/godot-core/src/builtin/string/node_path.rs index 8f0117be7..8b5aafc09 100644 --- a/godot-core/src/builtin/string/node_path.rs +++ b/godot-core/src/builtin/string/node_path.rs @@ -5,13 +5,14 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use std::fmt; +use std::{fmt, ops}; use godot_ffi as sys; use godot_ffi::{ffi_methods, ExtVariantType, GdextBuild, GodotFfi}; use super::{GString, StringName}; use crate::builtin::inner; +use crate::meta::godot_range::GodotRange; /// A pre-parsed scene tree path. /// @@ -127,10 +128,25 @@ 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 upper bound is not defined `exclusive_end` will span to the end of the `NodePath`. /// - /// 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)`. + /// # Example + /// ```no_run + /// # use godot::builtin::NodePath; + /// assert_eq!(NodePath::from("path/to/Node:with:props").subpath(-1..), ":props".into()); + /// ``` + /// + /// If either `begin` or `exclusive_end` are negative, they will be relative to the end of the `NodePath`. + /// + /// # Example + /// ```no_run + /// # use godot::builtin::NodePath; + /// let path = NodePath::from("path/to/Node:with:props"); + /// let total_count = path.get_total_count() as i32; + /// // Both are equal to "path/to/Node". + /// assert_eq!(path.subpath(0..-2), path.subpath(0..total_count - 2)); + /// + /// ``` /// /// _Godot equivalent: `slice`_ /// @@ -140,16 +156,17 @@ impl NodePath { // 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 ops::RangeBounds) -> NodePath { + let (from, exclusive_end) = range.to_godot_range_fromto(); // 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 +176,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! { diff --git a/godot-core/src/builtin/string/string_macros.rs b/godot-core/src/builtin/string/string_macros.rs index ade9e43f2..1b85f2e74 100644 --- a/godot-core/src/builtin/string/string_macros.rs +++ b/godot-core/src/builtin/string/string_macros.rs @@ -81,13 +81,15 @@ 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, range: impl std::ops::RangeBounds) -> usize { - let (from, to) = super::to_godot_fromto(range); + use $crate::meta::godot_range::GodotRange; + let (from, to) = range.to_godot_range_fromto_checked(0); 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, range: impl std::ops::RangeBounds) -> usize { - let (from, to) = super::to_godot_fromto(range); + use $crate::meta::godot_range::GodotRange; + let (from, to) = range.to_godot_range_fromto_checked(0); self.as_inner().countn(what, from, to) as usize } @@ -121,7 +123,8 @@ 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) -> GString { - let (from, len) = super::to_godot_fromlen_neg1(range); + use $crate::meta::godot_range::GodotRange; + let (from, len) = range.to_godot_range_fromlen(-1); self.as_inner().substr(from, len) } @@ -163,7 +166,11 @@ 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) -> GString { - let (from, len) = super::to_godot_fromlen_i32max(range); + use $crate::meta::godot_range::GodotRange; + // Unbounded upper bounds are represented by `i32::MAX` instead of `i64::MAX`, + // since Godot treats some indexes as 32-bit despite being declared `i64` in GDExtension API. + let (from, len) = range.to_godot_range_fromlen(i32::MAX as i64); + self.as_inner().erase(from, len) } diff --git a/godot-core/src/meta/godot_range.rs b/godot-core/src/meta/godot_range.rs new file mode 100644 index 000000000..805a137d0 --- /dev/null +++ b/godot-core/src/meta/godot_range.rs @@ -0,0 +1,84 @@ +/* + * Copyright (c) godot-rust; Bromeon and contributors. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +use std::fmt::{Debug, Display}; +use std::ops; + +/// Trait easing converting Rust ranges to values expected by Godot. +/// +/// Note: Unbounded upper bounds must be represented by `i32::MAX` instead of `i64::MAX`, +/// since Godot treats some indexes as 32-bit despite being declared `i64` in GDExtension API. +pub(crate) trait GodotRange { + /// Returns a tuple of `(from, to)` from a Rust range. + fn to_godot_range_fromto(&self) -> (i64, Option); + + /// Returns a tuple of `(from, to)` from a Rust range. + /// + /// Unbounded upper bound will be represented by `from = default_unbounded_upper`. + /// + /// # Panics + /// In debug mode, when `from` > `to`. + fn to_godot_range_fromto_checked(&self, default_unbounded_upper: i64) -> (i64, i64) { + match self.to_godot_range_fromto() { + (from, Some(to)) => { + debug_assert!(from <= to, "range: start ({from}) > end ({to})"); + (from, to) + } + (from, None) => (from, default_unbounded_upper), + } + } + + /// Returns a tuple of `(from, len)` from a Rust range. + /// + /// Unbounded upper bounds are represented by `len = default_unbounded_upper`. + /// + /// # Panics + /// In debug mode, when `from > to` (i.e. `len < 0`). + fn to_godot_range_fromlen(&self, default_unbounded_upper: i64) -> (i64, i64) { + match self.to_godot_range_fromto() { + (from, Some(to)) => { + debug_assert!(from <= to, "range: start ({from}) > end ({to})"); + (from, to - from) + } + (from, None) => (from, default_unbounded_upper), + } + } +} + +// Blanket implementation for any range which can be converted to (i64, i64). +// Supports both `RangeBounds` (e.g. `GString::count(..)`) and `RangeBounds` (e.g. `array.subarray_deep(-1..-5..)`) . +// `RangeBounds` should be used for ranges which can't/shouldn't be negative. +// `RangeBounds` for any range which supports negative indices (mostly methods related to Array). +impl GodotRange for R +where + R: ops::RangeBounds, + i64: TryFrom, + T: Copy + Display, + >::Error: Debug, +{ + fn to_godot_range_fromto(&self) -> (i64, Option) { + let from = match self.start_bound() { + ops::Bound::Included(&n) => i64::try_from(n).unwrap(), + ops::Bound::Excluded(&n) => i64::try_from(n).unwrap() + 1, + ops::Bound::Unbounded => 0, + }; + + let to = match self.end_bound() { + ops::Bound::Included(&n) => { + let to = i64::try_from(n).unwrap() + 1; + Some(to) + } + ops::Bound::Excluded(&n) => { + let to = i64::try_from(n).unwrap(); + Some(to) + } + ops::Bound::Unbounded => None, + }; + + (from, to) + } +} diff --git a/godot-core/src/meta/mod.rs b/godot-core/src/meta/mod.rs index f792c2848..5978bbb3a 100644 --- a/godot-core/src/meta/mod.rs +++ b/godot-core/src/meta/mod.rs @@ -56,6 +56,7 @@ mod uniform_object_deref; pub(crate) mod sealed; pub mod error; +pub(crate) mod godot_range; pub mod inspect; // Public re-exports diff --git a/godot-core/src/tools/gfile.rs b/godot-core/src/tools/gfile.rs index c0faa2f0e..4b6748eae 100644 --- a/godot-core/src/tools/gfile.rs +++ b/godot-core/src/tools/gfile.rs @@ -747,7 +747,7 @@ impl Write for GFile { fn write(&mut self, buf: &[u8]) -> std::io::Result { self.pack_into_write_buffer(buf); self.fa - .store_buffer(&self.write_buffer.subarray(0, buf.len())); + .store_buffer(&self.write_buffer.subarray(0..buf.len() as i32)); self.clear_file_length(); self.check_error()?; diff --git a/itest/rust/src/builtin_tests/containers/array_test.rs b/itest/rust/src/builtin_tests/containers/array_test.rs index 4855c2fbd..b1c75c2a0 100644 --- a/itest/rust/src/builtin_tests/containers/array_test.rs +++ b/itest/rust/src/builtin_tests/containers/array_test.rs @@ -136,14 +136,28 @@ fn array_duplicate_deep() { } #[itest] +#[allow(clippy::reversed_empty_ranges)] fn array_subarray_shallow() { let array = array![0, 1, 2, 3, 4, 5]; - let slice = array.subarray_shallow(5, 1, Some(-2)); + + let normal_slice = array.subarray_shallow(4..=5, None); + assert_eq!(normal_slice, array![4, 5]); + + let slice = array.subarray_shallow(5..1, Some(-2)); assert_eq!(slice, array![5, 3]); + let negative_slice = array.subarray_shallow(-1..-5, Some(-2)); + assert_eq!(negative_slice, array![5, 3]); + + let clamped_slice = array.subarray_shallow(100..-1, None); + assert_eq!(clamped_slice, array![]); + + let other_clamped_slice = array.subarray_shallow(5.., Some(2)); + assert_eq!(other_clamped_slice, array![5]); + let subarray = array![2, 3]; let array = varray![1, subarray]; - let slice = array.subarray_shallow(1, 2, None); + let slice = array.subarray_shallow(1..2, None); Array::::try_from_variant(&slice.at(0)) .unwrap() .set(0, 4); @@ -151,14 +165,28 @@ fn array_subarray_shallow() { } #[itest] +#[allow(clippy::reversed_empty_ranges)] fn array_subarray_deep() { let array = array![0, 1, 2, 3, 4, 5]; - let slice = array.subarray_deep(5, 1, Some(-2)); + + let normal_slice = array.subarray_deep(4..=5, None); + assert_eq!(normal_slice, array![4, 5]); + + let slice = array.subarray_deep(5..1, Some(-2)); assert_eq!(slice, array![5, 3]); + let negative_slice = array.subarray_deep(-1..-5, Some(-2)); + assert_eq!(negative_slice, array![5, 3]); + + let clamped_slice = array.subarray_deep(100..-1, None); + assert_eq!(clamped_slice, array![]); + + let other_clamped_slice = array.subarray_deep(5.., Some(2)); + assert_eq!(other_clamped_slice, array![5]); + let subarray = array![2, 3]; let array = varray![1, subarray]; - let slice = array.subarray_deep(1, 2, None); + let slice = array.subarray_deep(1..2, None); Array::::try_from_variant(&slice.at(0)) .unwrap() .set(0, 4); diff --git a/itest/rust/src/builtin_tests/containers/packed_array_test.rs b/itest/rust/src/builtin_tests/containers/packed_array_test.rs index 00e06a647..a6c4ef01f 100644 --- a/itest/rust/src/builtin_tests/containers/packed_array_test.rs +++ b/itest/rust/src/builtin_tests/containers/packed_array_test.rs @@ -446,14 +446,20 @@ fn packed_array_extend_array() { fn packed_array_subarray() { let array = PackedArray::from([10, 20, 30, 40, 50]); - let sub = array.subarray(1, 4); + let sub = array.subarray(1..4); assert_eq!(sub.as_slice(), &[20, 30, 40]); - let sub_empty = array.subarray(2, 2); + let endless = array.subarray(2..); + assert_eq!(endless.as_slice(), &[30, 40, 50]); + + let negative_sub = array.subarray(-4..-2); + assert_eq!(negative_sub.as_slice(), &[20, 30]); + + let sub_empty = array.subarray(2..2); assert_eq!(sub_empty.len(), 0); // Half-open range: end index is clamped to len. - let sub_clamped = array.subarray(3, 100); + let sub_clamped = array.subarray(3..100); assert_eq!(sub_clamped.as_slice(), &[40, 50]); } diff --git a/itest/rust/src/builtin_tests/string/node_path_test.rs b/itest/rust/src/builtin_tests/string/node_path_test.rs index 19fb45fbb..f057ca965 100644 --- a/itest/rust/src/builtin_tests/string/node_path_test.rs +++ b/itest/rust/src/builtin_tests/string/node_path_test.rs @@ -88,22 +88,23 @@ fn node_path_with_null() { #[itest] #[cfg(since_api = "4.3")] +#[allow(clippy::reversed_empty_ranges)] fn node_path_subpath() { let path = NodePath::from("path/to/Node:with:props"); let parts = path.get_name_count() + path.get_subname_count(); - assert_eq!(path.subpath(0, 1), "path".into()); - assert_eq!(path.subpath(1, 2), "to".into()); - assert_eq!(path.subpath(2, 3), "Node".into()); - assert_eq!(path.subpath(3, 4), ":with".into()); - assert_eq!(path.subpath(4, 5), ":props".into()); - - assert_eq!(path.subpath(1, -1), "to/Node:with".into()); - assert_eq!(path.subpath(1, parts as i32 - 1), "to/Node:with".into()); - assert_eq!(path.subpath(0, -2), "path/to/Node".into()); - assert_eq!(path.subpath(-3, -1), "Node:with".into()); - assert_eq!(path.subpath(-2, i32::MAX), ":with:props".into()); - assert_eq!(path.subpath(-1, i32::MAX), ":props".into()); + assert_eq!(path.subpath(0..1), "path".into()); + assert_eq!(path.subpath(1..2), "to".into()); + assert_eq!(path.subpath(2..3), "Node".into()); + assert_eq!(path.subpath(3..4), ":with".into()); + assert_eq!(path.subpath(4..5), ":props".into()); + + assert_eq!(path.subpath(1..-1), "to/Node:with".into()); + assert_eq!(path.subpath(1..parts as i32 - 1), "to/Node:with".into()); + assert_eq!(path.subpath(0..-2), "path/to/Node".into()); + assert_eq!(path.subpath(-3..-1), "Node:with".into()); + assert_eq!(path.subpath(-2..), ":with:props".into()); + assert_eq!(path.subpath(-1..), ":props".into()); } #[itest] From 0c9fdc41771def237ef115b84e6a8aba3440d7de Mon Sep 17 00:00:00 2001 From: Yarvin Date: Wed, 17 Sep 2025 18:05:00 +0200 Subject: [PATCH 2/2] Create `SignedRange` trait. Add `wrapped` utility function to handle signed ranges. --- godot-core/src/builtin/collections/array.rs | 97 +++++--------- .../src/builtin/collections/packed_array.rs | 26 ++-- .../collections/packed_array_element.rs | 9 +- godot-core/src/builtin/string/node_path.rs | 35 ++--- .../src/builtin/string/string_macros.rs | 15 +-- godot-core/src/meta/godot_range.rs | 84 ------------ godot-core/src/meta/mod.rs | 3 +- godot-core/src/meta/signed_range.rs | 121 ++++++++++++++++++ godot-core/src/tools/gfile.rs | 2 +- .../builtin_tests/containers/array_test.rs | 16 ++- .../containers/packed_array_test.rs | 4 +- .../builtin_tests/string/node_path_test.rs | 16 ++- 12 files changed, 219 insertions(+), 209 deletions(-) delete mode 100644 godot-core/src/meta/godot_range.rs create mode 100644 godot-core/src/meta/signed_range.rs diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index f74e1377b..8dbf439f2 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -7,7 +7,6 @@ use std::cell::OnceCell; use std::marker::PhantomData; -use std::ops::RangeBounds; use std::{cmp, fmt}; use godot_ffi as sys; @@ -16,7 +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::godot_range::GodotRange; +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, @@ -110,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 @@ -530,91 +558,34 @@ impl Array { /// 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 either `begin` or `end` are negative, their value is relative to the end of the array. - /// - /// # Example - /// ```no_run - /// # use godot::builtin::array; - /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_shallow(-1..-5, None), array![5, 3]); - /// ``` - /// - /// If `end` is not specified, the range spans through whole array. - /// - /// # Example - /// ```no_run - /// # use godot::builtin::array; - /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_shallow(1.., None), array![1, 2, 3, 4, 5]); - /// ``` - /// - /// If specified, `step` is the relative index between source elements. It can be negative, - /// in which case `begin` must be higher than `end`. - /// - /// # Example - /// ```no_run - /// # use godot::builtin::array; - /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_shallow(-1..-5, Some(-2)), array![5, 3]); - /// ``` - /// /// 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")] - pub fn subarray_shallow(&self, range: impl RangeBounds, step: Option) -> Self { + pub fn subarray_shallow(&self, range: impl SignedRange, step: Option) -> 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 either `begin` or `end` are negative, their value is relative to the end of the array. - /// - /// # Example - /// ```no_run - /// # use godot::builtin::array; - /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_deep(-1..-5, None), array![5, 3]); - /// ``` - /// - /// If `end` is not specified, the range spans through whole array. - /// - /// # Example - /// ```no_run - /// # use godot::builtin::array; - /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_deep(1.., None), array![1, 2, 3, 4, 5]); - /// ``` - /// - /// If specified, `step` is the relative index between source elements. It can be negative, - /// in which case `begin` must be higher than `end`. - /// - /// # Example - /// ```no_run - /// # use godot::builtin::array; - /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_deep(-1..-5, Some(-2)), array![5, 3]); - /// ``` - /// /// 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")] - pub fn subarray_deep(&self, range: impl RangeBounds, step: Option) -> Self { + pub fn subarray_deep(&self, range: impl SignedRange, step: Option) -> Self { self.subarray_impl(range, step, true) } // Note: Godot will clamp values by itself. - fn subarray_impl(&self, range: impl GodotRange, step: Option, deep: bool) -> Self { + fn subarray_impl(&self, range: impl SignedRange, step: Option, deep: bool) -> Self { assert_ne!(step, Some(0), "subarray: step cannot be zero"); let step = step.unwrap_or(1); - let (begin, end) = range.to_godot_range_fromto(); - - // Unbounded upper bounds are represented by `i32::MAX` instead of `i64::MAX`, - // since Godot treats some indexes as 32-bit despite being declared `i64` in GDExtension API. + 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` immediately. diff --git a/godot-core/src/builtin/collections/packed_array.rs b/godot-core/src/builtin/collections/packed_array.rs index 432eef1a5..f39549dce 100644 --- a/godot-core/src/builtin/collections/packed_array.rs +++ b/godot-core/src/builtin/collections/packed_array.rs @@ -10,7 +10,6 @@ #![allow(clippy::result_unit_err)] use std::iter::FromIterator; -use std::ops::RangeBounds; use std::{fmt, ops, ptr}; use godot_ffi as sys; @@ -20,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}; @@ -228,31 +228,29 @@ impl PackedArray { /// Returns a sub-range `begin..end`, as a new packed array. /// /// 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]. /// - /// If either `begin` or `end` are negative, their value is relative to the end of the array. + /// # Usage + /// For negative indices, use [`wrapped()`](crate::meta::wrapped). /// - /// # Example /// ```no_run /// # use godot::builtin::PackedArray; + /// # use godot::meta::wrapped; /// let array = PackedArray::from([10, 20, 30, 40, 50]); - /// assert_eq!(array.subarray(-4..-2), PackedArray::from([20, 30])); - /// ``` /// - /// If `end` is not specified, the resulting subarray will span to the end of the array. + /// // 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])); /// - /// # Example - /// ```no_run - /// # use godot::builtin::PackedArray; - /// let array = PackedArray::from([10, 20, 30, 40, 50]); - /// assert_eq!(array.subarray(2..), PackedArray::from([30, 40, 50])); + /// // 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])); /// ``` /// - /// To obtain Rust slices, see [`as_slice`][Self::as_slice] and [`as_mut_slice`][Self::as_mut_slice]. - /// /// _Godot equivalent: `slice`_ #[doc(alias = "slice")] // Note: Godot will clamp values by itself. - pub fn subarray(&self, range: impl RangeBounds) -> Self { + pub fn subarray(&self, range: impl SignedRange) -> Self { T::op_slice(self.as_inner(), range) } diff --git a/godot-core/src/builtin/collections/packed_array_element.rs b/godot-core/src/builtin/collections/packed_array_element.rs index 246461892..324254a9c 100644 --- a/godot-core/src/builtin/collections/packed_array_element.rs +++ b/godot-core/src/builtin/collections/packed_array_element.rs @@ -4,13 +4,12 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use std::ops::RangeBounds; use sys::{interface_fn, GodotFfi, SysPtr}; use crate::builtin::collections::extend_buffer::{ExtendBuffer, ExtendBufferTrait}; use crate::builtin::PackedArray; -use crate::meta::godot_range::GodotRange; +use crate::meta::signed_range::SignedRange; use crate::meta::{CowArg, FromGodot, GodotType, ToGodot}; use crate::registry::property::builtin_type_string; use crate::{builtin, sys}; @@ -128,7 +127,7 @@ pub trait PackedArrayElement: GodotType + Clone + ToGodot + FromGodot { fn op_append_array(inner: Self::Inner<'_>, other: &PackedArray); #[doc(hidden)] - fn op_slice(inner: Self::Inner<'_>, range: impl RangeBounds) -> PackedArray; + fn op_slice(inner: Self::Inner<'_>, range: impl SignedRange) -> PackedArray; #[doc(hidden)] fn op_find(inner: Self::Inner<'_>, value: CowArg<'_, Self>, from: i64) -> i64; @@ -287,8 +286,8 @@ macro_rules! impl_packed_array_element { inner.append_array(other); } - fn op_slice(inner: Self::Inner<'_>, range: impl RangeBounds) -> PackedArray { - let (begin, end) = range.to_godot_range_fromto(); + fn op_slice(inner: Self::Inner<'_>, range: impl $crate::meta::signed_range::SignedRange) -> PackedArray { + let (begin, end) = range.signed(); inner.slice(begin, end.unwrap_or(i32::MAX as i64)) } diff --git a/godot-core/src/builtin/string/node_path.rs b/godot-core/src/builtin/string/node_path.rs index 8b5aafc09..b6207baca 100644 --- a/godot-core/src/builtin/string/node_path.rs +++ b/godot-core/src/builtin/string/node_path.rs @@ -5,14 +5,14 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use std::{fmt, ops}; +use std::fmt; use godot_ffi as sys; use godot_ffi::{ffi_methods, ExtVariantType, GdextBuild, GodotFfi}; use super::{GString, StringName}; use crate::builtin::inner; -use crate::meta::godot_range::GodotRange; +use crate::meta::signed_range::SignedRange; /// A pre-parsed scene tree path. /// @@ -128,36 +128,37 @@ 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]. - /// If upper bound is not defined `exclusive_end` will span to the end of the `NodePath`. /// - /// # Example - /// ```no_run - /// # use godot::builtin::NodePath; - /// assert_eq!(NodePath::from("path/to/Node:with:props").subpath(-1..), ":props".into()); - /// ``` - /// - /// If either `begin` or `exclusive_end` are negative, they will be relative to the end of the `NodePath`. + /// # Usage + /// For negative indices, use [`wrapped()`](crate::meta::wrapped). /// - /// # Example /// ```no_run /// # use godot::builtin::NodePath; + /// # use godot::meta::wrapped; /// let path = NodePath::from("path/to/Node:with:props"); - /// let total_count = path.get_total_count() as i32; - /// // Both are equal to "path/to/Node". - /// assert_eq!(path.subpath(0..-2), path.subpath(0..total_count - 2)); /// + /// // 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, range: impl ops::RangeBounds) -> NodePath { - let (from, exclusive_end) = range.to_godot_range_fromto(); + 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") { from diff --git a/godot-core/src/builtin/string/string_macros.rs b/godot-core/src/builtin/string/string_macros.rs index 1b85f2e74..73035e004 100644 --- a/godot-core/src/builtin/string/string_macros.rs +++ b/godot-core/src/builtin/string/string_macros.rs @@ -81,15 +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, range: impl std::ops::RangeBounds) -> usize { - use $crate::meta::godot_range::GodotRange; - let (from, to) = range.to_godot_range_fromto_checked(0); + 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, range: impl std::ops::RangeBounds) -> usize { - use $crate::meta::godot_range::GodotRange; - let (from, to) = range.to_godot_range_fromto_checked(0); + let (from, to) = $crate::meta::signed_range::to_godot_range_fromto(range); self.as_inner().countn(what, from, to) as usize } @@ -123,8 +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) -> GString { - use $crate::meta::godot_range::GodotRange; - let (from, len) = range.to_godot_range_fromlen(-1); + let (from, len) = $crate::meta::signed_range::to_godot_range_fromlen(range, -1); self.as_inner().substr(from, len) } @@ -166,11 +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) -> GString { - use $crate::meta::godot_range::GodotRange; - // Unbounded upper bounds are represented by `i32::MAX` instead of `i64::MAX`, - // since Godot treats some indexes as 32-bit despite being declared `i64` in GDExtension API. - let (from, len) = range.to_godot_range_fromlen(i32::MAX as i64); - + let (from, len) = $crate::meta::signed_range::to_godot_range_fromlen(range, i32::MAX as i64); self.as_inner().erase(from, len) } diff --git a/godot-core/src/meta/godot_range.rs b/godot-core/src/meta/godot_range.rs deleted file mode 100644 index 805a137d0..000000000 --- a/godot-core/src/meta/godot_range.rs +++ /dev/null @@ -1,84 +0,0 @@ -/* - * Copyright (c) godot-rust; Bromeon and contributors. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. - */ - -use std::fmt::{Debug, Display}; -use std::ops; - -/// Trait easing converting Rust ranges to values expected by Godot. -/// -/// Note: Unbounded upper bounds must be represented by `i32::MAX` instead of `i64::MAX`, -/// since Godot treats some indexes as 32-bit despite being declared `i64` in GDExtension API. -pub(crate) trait GodotRange { - /// Returns a tuple of `(from, to)` from a Rust range. - fn to_godot_range_fromto(&self) -> (i64, Option); - - /// Returns a tuple of `(from, to)` from a Rust range. - /// - /// Unbounded upper bound will be represented by `from = default_unbounded_upper`. - /// - /// # Panics - /// In debug mode, when `from` > `to`. - fn to_godot_range_fromto_checked(&self, default_unbounded_upper: i64) -> (i64, i64) { - match self.to_godot_range_fromto() { - (from, Some(to)) => { - debug_assert!(from <= to, "range: start ({from}) > end ({to})"); - (from, to) - } - (from, None) => (from, default_unbounded_upper), - } - } - - /// Returns a tuple of `(from, len)` from a Rust range. - /// - /// Unbounded upper bounds are represented by `len = default_unbounded_upper`. - /// - /// # Panics - /// In debug mode, when `from > to` (i.e. `len < 0`). - fn to_godot_range_fromlen(&self, default_unbounded_upper: i64) -> (i64, i64) { - match self.to_godot_range_fromto() { - (from, Some(to)) => { - debug_assert!(from <= to, "range: start ({from}) > end ({to})"); - (from, to - from) - } - (from, None) => (from, default_unbounded_upper), - } - } -} - -// Blanket implementation for any range which can be converted to (i64, i64). -// Supports both `RangeBounds` (e.g. `GString::count(..)`) and `RangeBounds` (e.g. `array.subarray_deep(-1..-5..)`) . -// `RangeBounds` should be used for ranges which can't/shouldn't be negative. -// `RangeBounds` for any range which supports negative indices (mostly methods related to Array). -impl GodotRange for R -where - R: ops::RangeBounds, - i64: TryFrom, - T: Copy + Display, - >::Error: Debug, -{ - fn to_godot_range_fromto(&self) -> (i64, Option) { - let from = match self.start_bound() { - ops::Bound::Included(&n) => i64::try_from(n).unwrap(), - ops::Bound::Excluded(&n) => i64::try_from(n).unwrap() + 1, - ops::Bound::Unbounded => 0, - }; - - let to = match self.end_bound() { - ops::Bound::Included(&n) => { - let to = i64::try_from(n).unwrap() + 1; - Some(to) - } - ops::Bound::Excluded(&n) => { - let to = i64::try_from(n).unwrap(); - Some(to) - } - ops::Bound::Unbounded => None, - }; - - (from, to) - } -} diff --git a/godot-core/src/meta/mod.rs b/godot-core/src/meta/mod.rs index 5978bbb3a..7b37e79ce 100644 --- a/godot-core/src/meta/mod.rs +++ b/godot-core/src/meta/mod.rs @@ -56,8 +56,8 @@ mod uniform_object_deref; pub(crate) mod sealed; pub mod error; -pub(crate) mod godot_range; pub mod inspect; +pub(crate) mod signed_range; // Public re-exports pub use args::*; @@ -71,6 +71,7 @@ pub use property_info::{PropertyHintInfo, PropertyInfo}; pub use signature::trace; #[doc(hidden)] pub use signature::*; +pub use signed_range::{wrapped, SignedRange}; pub use traits::{ArrayElement, GodotType, PackedArrayElement}; pub use uniform_object_deref::UniformObjectDeref; diff --git a/godot-core/src/meta/signed_range.rs b/godot-core/src/meta/signed_range.rs new file mode 100644 index 000000000..d5aa318ea --- /dev/null +++ b/godot-core/src/meta/signed_range.rs @@ -0,0 +1,121 @@ +/* + * Copyright (c) godot-rust; Bromeon and contributors. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +use std::collections::Bound; +use std::ops::RangeBounds; + +/// Range which can be wrapped. +/// +/// Constructed with [`wrapped()`] utility function. +struct WrappedRange { + lower_bound: i64, + upper_bound: Option, +} + +/// Accepts negative bounds, interpreted relative to the end of the collection. +/// +/// ## Examples +/// ```no_run +/// # use godot::meta::wrapped; +/// wrapped(1..-2); // from 1 to len-2 +/// wrapped(..-2); // from 0 to len-2 +/// wrapped(-3..); // from len-3 to end +/// wrapped(-4..3); // from len-4 to 3 +/// ``` +pub fn wrapped(signed_range: impl RangeBounds) -> impl SignedRange +where + T: Copy + Into, +{ + let lower_bound = lower_bound(signed_range.start_bound().map(|v| (*v).into())).unwrap_or(0); + let upper_bound = upper_bound(signed_range.end_bound().map(|v| (*v).into())); + + WrappedRange { + lower_bound, + upper_bound, + } +} + +fn lower_bound(bound: Bound) -> Option { + match bound { + Bound::Included(n) => Some(n), + Bound::Excluded(n) => Some(n + 1), + Bound::Unbounded => None, + } +} + +fn upper_bound(bound: Bound) -> Option { + match bound { + Bound::Included(n) => Some(n + 1), + Bound::Excluded(n) => Some(n), + Bound::Unbounded => None, + } +} + +mod sealed { + pub trait SealedRange {} +} + +/// Trait supporting regular `usize` ranges, as well as negative indices. +/// +/// If a lower or upper bound is negative, then its value is relative to the end of the given collection. \ +/// Use the [`wrapped()`] utility function to construct such ranges. +pub trait SignedRange: sealed::SealedRange { + /// Returns a tuple of `(from, to)` from a Rust range. + /// Unbounded upper range is represented by `None`. + // Note: in some cases unbounded upper bounds should be represented by `i32::MAX` instead of `i64::MAX`, + // since Godot treats some indexes as 32-bit despite being declared as `i64` in GDExtension API. + #[doc(hidden)] + fn signed(&self) -> (i64, Option); +} + +impl sealed::SealedRange for WrappedRange {} +impl SignedRange for WrappedRange { + fn signed(&self) -> (i64, Option) { + (self.lower_bound, self.upper_bound) + } +} + +impl sealed::SealedRange for R where R: RangeBounds {} +impl SignedRange for R +where + R: RangeBounds, +{ + fn signed(&self) -> (i64, Option) { + let lower_bound = lower_bound(self.start_bound().map(|v| *v as i64)).unwrap_or(0); + let upper_bound = upper_bound(self.end_bound().map(|v| *v as i64)); + + (lower_bound, upper_bound) + } +} + +/// Returns a tuple of `(from, to)` from a Rust range. +/// +/// # Panics +/// In debug mode, when from > to. +pub(crate) fn to_godot_range_fromto(range: impl SignedRange) -> (i64, i64) { + match range.signed() { + (from, Some(to)) => { + debug_assert!(from <= to, "range: start ({from}) > end ({to})"); + (from, to) + } + (from, None) => (from, 0), + } +} + +/// Returns a tuple of `(from, len)` from a Rust range. +/// +/// # Panics +/// In debug mode, when from > to. +pub(crate) fn to_godot_range_fromlen(range: impl SignedRange, unbounded: i64) -> (i64, i64) { + match range.signed() { + (from, Some(to)) => { + debug_assert!(from <= to, "range: start ({from}) > end ({to})"); + (from, to - from) + } + (from, None) => (from, unbounded), + } +} diff --git a/godot-core/src/tools/gfile.rs b/godot-core/src/tools/gfile.rs index 4b6748eae..5b97a5945 100644 --- a/godot-core/src/tools/gfile.rs +++ b/godot-core/src/tools/gfile.rs @@ -747,7 +747,7 @@ impl Write for GFile { fn write(&mut self, buf: &[u8]) -> std::io::Result { self.pack_into_write_buffer(buf); self.fa - .store_buffer(&self.write_buffer.subarray(0..buf.len() as i32)); + .store_buffer(&self.write_buffer.subarray(0..buf.len())); self.clear_file_length(); self.check_error()?; diff --git a/itest/rust/src/builtin_tests/containers/array_test.rs b/itest/rust/src/builtin_tests/containers/array_test.rs index b1c75c2a0..95c6c994a 100644 --- a/itest/rust/src/builtin_tests/containers/array_test.rs +++ b/itest/rust/src/builtin_tests/containers/array_test.rs @@ -5,7 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use godot::meta::ElementType; +use godot::meta::{wrapped, ElementType}; use godot::prelude::*; use crate::framework::{assert_match, create_gdscript, expect_panic, itest}; @@ -146,10 +146,13 @@ fn array_subarray_shallow() { let slice = array.subarray_shallow(5..1, Some(-2)); assert_eq!(slice, array![5, 3]); - let negative_slice = array.subarray_shallow(-1..-5, Some(-2)); + let negative_slice = array.subarray_shallow(wrapped(-1..-5), Some(-2)); assert_eq!(negative_slice, array![5, 3]); - let clamped_slice = array.subarray_shallow(100..-1, None); + let other_negative_slice = array.subarray_shallow(wrapped(-1..3), Some(-1)); + assert_eq!(other_negative_slice, array![5, 4]); + + let clamped_slice = array.subarray_shallow(wrapped(100..-1), None); assert_eq!(clamped_slice, array![]); let other_clamped_slice = array.subarray_shallow(5.., Some(2)); @@ -175,10 +178,13 @@ fn array_subarray_deep() { let slice = array.subarray_deep(5..1, Some(-2)); assert_eq!(slice, array![5, 3]); - let negative_slice = array.subarray_deep(-1..-5, Some(-2)); + let negative_slice = array.subarray_deep(wrapped(-1..-5), Some(-2)); assert_eq!(negative_slice, array![5, 3]); - let clamped_slice = array.subarray_deep(100..-1, None); + let other_negative_slice = array.subarray_deep(wrapped(-1..3), Some(-1)); + assert_eq!(other_negative_slice, array![5, 4]); + + let clamped_slice = array.subarray_deep(wrapped(100..-1), None); assert_eq!(clamped_slice, array![]); let other_clamped_slice = array.subarray_deep(5.., Some(2)); diff --git a/itest/rust/src/builtin_tests/containers/packed_array_test.rs b/itest/rust/src/builtin_tests/containers/packed_array_test.rs index a6c4ef01f..b8a2fe973 100644 --- a/itest/rust/src/builtin_tests/containers/packed_array_test.rs +++ b/itest/rust/src/builtin_tests/containers/packed_array_test.rs @@ -11,7 +11,7 @@ use godot::builtin::{ Variant, Vector2, Vector3, Vector4, }; use godot::global::godot_str; -use godot::meta::{owned_into_arg, ref_to_arg, PackedArrayElement, ToGodot}; +use godot::meta::{owned_into_arg, ref_to_arg, wrapped, PackedArrayElement, ToGodot}; use crate::framework::{expect_panic, itest}; @@ -452,7 +452,7 @@ fn packed_array_subarray() { let endless = array.subarray(2..); assert_eq!(endless.as_slice(), &[30, 40, 50]); - let negative_sub = array.subarray(-4..-2); + let negative_sub = array.subarray(wrapped(-4..-2)); assert_eq!(negative_sub.as_slice(), &[20, 30]); let sub_empty = array.subarray(2..2); diff --git a/itest/rust/src/builtin_tests/string/node_path_test.rs b/itest/rust/src/builtin_tests/string/node_path_test.rs index f057ca965..38ec7a80d 100644 --- a/itest/rust/src/builtin_tests/string/node_path_test.rs +++ b/itest/rust/src/builtin_tests/string/node_path_test.rs @@ -8,6 +8,7 @@ use std::collections::HashSet; use godot::builtin::{GString, NodePath}; +use godot::meta::wrapped; use crate::framework::{expect_debug_panic_or_release_ok, itest}; @@ -99,12 +100,15 @@ fn node_path_subpath() { assert_eq!(path.subpath(3..4), ":with".into()); assert_eq!(path.subpath(4..5), ":props".into()); - assert_eq!(path.subpath(1..-1), "to/Node:with".into()); - assert_eq!(path.subpath(1..parts as i32 - 1), "to/Node:with".into()); - assert_eq!(path.subpath(0..-2), "path/to/Node".into()); - assert_eq!(path.subpath(-3..-1), "Node:with".into()); - assert_eq!(path.subpath(-2..), ":with:props".into()); - assert_eq!(path.subpath(-1..), ":props".into()); + assert_eq!(path.subpath(wrapped(1..-1)), "to/Node:with".into()); + assert_eq!( + path.subpath(wrapped(1..parts as i32 - 1)), + "to/Node:with".into() + ); + assert_eq!(path.subpath(wrapped(0..-2)), "path/to/Node".into()); + assert_eq!(path.subpath(wrapped(-3..-1)), "Node:with".into()); + assert_eq!(path.subpath(wrapped(-2..)), ":with:props".into()); + assert_eq!(path.subpath(wrapped(-1..)), ":props".into()); } #[itest]