-
-
Notifications
You must be signed in to change notification settings - Fork 267
Add SignedRange for negative indices in range ops
#1300
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 SignedRange for negative indices in range ops
#1300
Conversation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1300 |
|
Thanks! I do wonder a bit regarding overall consistency, some things may have changed since that
It's very hard for users to know when to use which. It also seems like there hasn't been much demand for this feature? An option I see: for most operations, use
See also: |
edce765 to
629ea9f
Compare
|
I ended with I had some doubts, since all functions involved are just some calls to the Godot Engine, but being able to specify bounds such as I also updated docs, no idea how I missed them last time 😅.
Because they all were called the same and probably nobody noticed a difference – similarly a function such as |
e42d9c7 to
148b035
Compare
102ff56 to
aa4be7b
Compare
godot-core/src/builtin/mod.rs
Outdated
| /// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't public therefore docstring is fine (easier to spot in IDE).
Godot indirectly informs that one should use i32 in docs, by providing default values. For example: https://docs.godotengine.org/en/stable/classes/class_nodepath.html#class-nodepath-method-slice
80ff06d to
e436a16
Compare
c6d7cec to
9864a5d
Compare
Bromeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the nice unification!
The direct range support for i32 is very nice for literals. There is however a number of index/size APIs based on usize, as mentioned in #1300 (comment), which (in non-literal cases) now need to be written as a as i32..b as i32 🤔
Should we maybe allow something like
// usize
a.subarray(2..3, None);
// i32
a.subarray(wrap(-2..), None);where wrap would take RangeBounds<i32> and convert to RangeBounds<usize>? That way we can support both conventions: the "rusty" one as the default, and the Godot one via wrap (or another short name).
godot-core/src/meta/godot_range.rs
Outdated
| // Blanket implementation for any range which can be converted to (i64, i64). | ||
| impl<T, R> GodotRange<T> for R | ||
| where | ||
| R: ops::RangeBounds<T>, | ||
| i64: TryFrom<T>, | ||
| T: Copy + Display, | ||
| <T as TryInto<i64>>::Error: Debug, | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit overengineered, why not RangeBounds<i32> or RangeBounds<i64>? We can always extend if necessary, but I'd prefer Simplicity as per API philosophy.
Does this need to be a trait at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I want to keep all range-related operations in one place, covering both Range<usize> (for strings and whatnot) and Range<i32> (for arrays/subarrays which can take negative values). They both use the same logic and I avoid changes in string-related API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many signs of premature abstraction here:
- only 2 impls, not much duplication in the first place
- logic like
.unwrap_or(i32::MAX as i64)is now duplicated on the call site, because the abstraction is too generic to_godot_range_fromto_checkedis only ever invoked with0- both
to_godot_range_fromto_checkedandto_godot_range_fromlenare only monomorphized forusize, not fori32
It seems to me that a handful of functions would do the job easier (without 4 bounds).
The only function that seems to be reused for both types is to_godot_range_fromto, and that could still remain generic as a function without the entire trait. Also, bounds can be simplified into:
where
R: RangeBounds<T>,
T: Copy + Display + TryInto<i64, Error: Debug>,There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, makes sense 👍
| /// | ||
| /// # Example | ||
| /// ```no_run | ||
| /// # use godot_core::array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to use godot:: imports, even in commented-out lines (here godot::builtin::array).
godot-core/src/builtin/mod.rs
Outdated
| // ---------------------------------------------------------------------------------------------------------------------------------------------- | ||
| // utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // ---------------------------------------------------------------------------------------------------------------------------------------------- | |
| // utils | |
| // ---------------------------------------------------------------------------------------------------------------------------------------------- | |
| // Conversion functions |
That's why I went, finally, for a trait – it provides blanket implementation both for i.e. following work // `count` uses - and used - `Range<usize>`, no changes here.
assert_eq!(s.count("en", 6..=6), 0);
// Same for `erase`.
assert_eq!(s.erase(2..=2), "Helo World".into());
// And `substr`.
assert_eq!(string.substr(2..=4), "abl".into());
// Unlike arrays which, from now on, use RangeBounds<i32>.
let negative_slice = array.subarray_deep(-1..-5, Some(-2));(Range instead of RE: Wrap – it sounds very stupid, but I think |
9864a5d to
2d2d10b
Compare
But |
|
Something else. If I do this in GDScript: var array = [0, 1, 2, 3, 4, 5]
var negative_slice = array.slice(-1, 3, -1)
print(negative_slice)I get result: Is this supported? Either way, I think we're up for some surprises. The underlying problem is that we're stuffing something into Rust ranges that aren't real ranges in the Rust sense. Rust ranges support neither wrap-around nor step. I do wonder if such negative indexes should be passed more explicitly when occurring in argument position 🤔 |
Yep, that's by design – typesystem makes sure that you can't pass negative values when positive ones are expected. On another hand – I'm not sure if it is something we should shield against and one more debug assert would be enough, since it is not something one encounters often enough (and Godot will emit proper error on its side) 🤔. I think I'll expose GodotRange if we decide to go with the ranges.
RangeBounds have two purposes in Rust – they are either an argument (like for a
Yep! let other_negative_slice = array.subarray_deep(-1..3, Some(-1));
assert_eq!(other_negative_slice, array![5, 4]);In general there are two options: I also thought about something along the lines of // Upper range == 2.
foo(1, 2);
// Upper range == Unbounded
foo(1, None);but it is a bit too confusing (not sure if I've ever seen such API). |
|
It seems to me we have 3 sorts of ranges:
Type 1 should already be covered with Type 2 should ideally support all the trait SignedRange { ... }
impl<T> SignedRange for T
where T: RangeBounds<usize> { ... }
impl SignedRange for WrappedRange { ... }
// with utility construction
fn wrapped(from: i32, to_excl: i32) -> WrappedRange { ... }Would something like that work? Or do we need half-open ranges for negative numbers too? Then we could also accept fn wrapped(signed_range: impl RangeBounds<i32>) -> WrappedRange { ... } |
…ected by Godot. Support negative indices for Array. Fix Array docs.
cd022d5 to
095de28
Compare
|
I was messing with
foo(1..999);
foo(wrapped(-10..33));
foo((-10, 33));
foo(wrapped(-10..));
foo((-10,));🤔. I've found it a little too janky. Logic like I was hunting for similar API, and haven't found anything – which is weird, since negative range bounds are fully legal in rust. (on a side note rust ranges are absurdly annoying to work with) |
Bromeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, starts to look great! 🙂
Logic like
.unwrap_or(i32::MAX as i64)is executed on the call site since setting proper upper range bounds is its responsibility.
With "call site" you mean godot-rust implementation and not user code, right? It looks like we can abstract this inconsistency from the user, which is very nice 👍
| /// The values of `begin` (inclusive) and `end` (exclusive) will be clamped to the array size. | ||
| /// | ||
| /// For negative indices use [`wrapped`](crate::meta::wrapped). | ||
| /// If either `begin` or `end` are negative, their value is relative to the end of the array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// If either `begin` or `end` are negative, their value is relative to the end of the array. | |
| /// If either `begin` or `end` is negative, its value is relative to the end of the array. |
Singular, also in all other comments like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense (if one out of the bunch is… then its – one being negative doesn't change the behavior of the another), I copy-pasted it from https://docs.godotengine.org/en/stable/classes/class_array.html#class-array-method-slice 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, maybe we should fix that too 😀
| /// # Example | ||
| /// ```no_run | ||
| /// # use godot::builtin::array; | ||
| /// # use godot::meta::wrapped; | ||
| /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_deep(wrapped(-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; | ||
| /// # use godot::meta::wrapped; | ||
| /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_deep(wrapped(-1..-5), Some(-2)), array![5, 3]); | ||
| /// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's worth duplicating the examples, I would just link to subarray_shallow() here. Otherwise it risks getting out-of-sync.
godot-core/src/meta/godot_range.rs
Outdated
| pub fn wrapped<T>(signed_range: impl RangeBounds<T>) -> impl SignedRange | ||
| where | ||
| T: Copy, | ||
| i64: From<T>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bounds are unusual, Into doesn't work here?
| pub fn wrapped<T>(signed_range: impl RangeBounds<T>) -> impl SignedRange | |
| where | |
| T: Copy, | |
| i64: From<T>, | |
| pub fn wrapped<T>(signed_range: impl RangeBounds<T>) -> impl SignedRange | |
| where | |
| T: Copy + Into<i64> |
Also, is Copy needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with into, but for some reason compiler demands i64: From<T> (while one should imply another >:[).
error[E0277]: the trait bound `i64: std::convert::From<T>` is not satisfied
--> godot-core/src/meta/godot_range.rs:30:70
|
30 | let lower_bound = lower_bound(signed_range.start_bound().map(|v| i64::from(*v))).unwrap_or(0);
| ^^^ the trait `std::convert::From<T>` is not implemented for `i64`
|
help: consider extending the `where` clause, but there might be an alternative better way to express this requirement
|
28 | T: Copy + Into<i64>, i64: std::convert::From<T>
| ++++++++++++++++++++++++++The workaround is to use into() which looks silly.
let lower_bound = lower_bound(signed_range.start_bound().map(|v| Into::<i64>::into(*v))).unwrap_or(0);
let upper_bound = upper_bound(signed_range.end_bound().map(|v| Into::<i64>::into(*v)));As for copy – i64 does value-to-value conversions, so we need to copy the bound value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You explicitly call From::from in the body though...
From implies Into, not the other way around 🙂
Can into() not be type-inferred if you add : i64 to the variable decls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But minor detail, leave From if it gets annoying, I was just not used to see it much in bounds...
godot-core/src/meta/godot_range.rs
Outdated
| /// Trait for ranges with negative bounds which can be used with the Godot API. | ||
| /// If any bound is negative then their value is relative to the end of the given collection. | ||
| pub trait SignedRange { | ||
| /// 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. | ||
| fn signed(&self) -> (i64, Option<i64>); | ||
| } | ||
|
|
||
| impl SignedRange for WrappedRange { | ||
| fn signed(&self) -> (i64, Option<i64>) { | ||
| (self.lower_bound, self.upper_bound) | ||
| } | ||
| } | ||
|
|
||
| impl<R> SignedRange for R | ||
| where | ||
| R: RangeBounds<usize>, | ||
| { | ||
| fn signed(&self) -> (i64, Option<i64>) { | ||
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapped() is a public function, but returns SignedRange which isn't part of the public API.
While not a huge issue, I wonder if it would hurt to expose the trait and #[doc(hidden)] its method? Can also be sealed for now (removing that would always be a backward-compatible change, but we can't hide a required method otherwise).
| let other_negative_slice = array.subarray_deep(wrapped(-1..3), Some(-1)); | ||
| assert_eq!(other_negative_slice, array![5, 4]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice to see that this is supported!
Answers my question from #1300 (comment).
| /// ```no_run | ||
| /// # use godot::builtin::array; | ||
| /// # use godot::meta::wrapped; | ||
| /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_shallow(wrapped(-1..-5), None), array![5, 3]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// assert_eq!(array![0, 1, 2, 3, 4, 5].subarray_shallow(wrapped(-1..-5), None), array![5, 3]); | |
| /// let arr = array![0, 1, 2, 3, 4, 5]; | |
| /// let sub = arr.subarray_deep(wrapped(-1..-5), None); | |
| /// assert_eq!(sub, array![5, 3]); |
A bit easier to understand (also for other examples like this).
You can maybe also combine the code snippets to one, then you don't need to redeclare the array 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to the top of the file instead. We can also use SignedRange trait docs for that 🤔.
3d8a8d5 to
66eef65
Compare
SignedRange for negative indices in range ops
Add `wrapped` utility function to handle signed ranges.
66eef65 to
0c9fdc4
Compare
|
Renamed |
The title says all, I guess – I resolved lagging TODOs before 0.4.