Provide feature parity for NodePath with Godot
#982
Conversation
|
We could also consider renaming some of the function names, but I don't know how that would be done. Any ideas are great! |
Bromeon
added
feature
There was a problem hiding this comment.
Thanks a lot for this, very nice addition! 💪
Some feedback below.
For indices, we should generally use usize instead of u32 -- this requires fewer conversions on user side, e.g. when iterating in a loop, comparing with len() etc.
I'm not 100% sure if get_name()/get_subname() should return Option, or just have a debug_assert. Now it's slightly more annoying to iterate -- without Option, it would be
let sprite_path = NodePath::from("../RigidBody2D/Sprite2D");
for i in 0..sprite_path.get_name_count() {
let name = sprite_path.get_name(i);
// do something
}and now it needs extra unwrap(), even though the bounds are already checked.
We could of course be very fancy and return an iterator, but there are still cases where random access is needed, so we'd end up providing a lot of functions for this.
Thoughts?
Since the logic is non-trivial for certain operations (slice and index access), it would be good to write short tests. You could e.g. use my PR's gstring_test.rs addition as inspiration. Let us know if you need any help!
| /// Returns the slice of the [`NodePath`] as a new [`NodePath`] | ||
| pub fn slice(&self, range: impl RangeBounds<i64>) -> NodePath { | ||
| self.as_inner().slice( | ||
| match range.start_bound() { | ||
| Excluded(&start) => { | ||
| if start == -1 { | ||
| // Default end from godot, since the start is excluded. | ||
| i32::MAX as i64 | ||
| } else { | ||
| start + 1 | ||
| } | ||
| } | ||
| Included(&start) => start, | ||
| Unbounded => 0, | ||
| }, | ||
| match range.end_bound() { | ||
| Excluded(&end) => end, | ||
| Included(&end) => { | ||
| if end == -1 { | ||
| // Default end from godot, since the end is excluded. | ||
| i32::MAX as i64 | ||
| } else { | ||
| end + 1 | ||
| } | ||
| } | ||
| // Default end from godot. | ||
| Unbounded => i32::MAX as i64, | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
As mentioned on Discord, I'm not sure if we should provide ranges and negative indices. These two are usually exclusive, and I'd say Rust users expect ranges to be monotonically increasing.
GDScript uses this signature:
NodePath slice(begin: int, end: int = 2147483647) const If you don't want to provide Option<i64> for the end parameter, you can always have two separate functions slice and slice_from or so.
There was a problem hiding this comment.
Alternatively, we don't provide negative indices, in which case you might be able to reuse to_fromlen_pair from my PR #980.
There was a problem hiding this comment.
Wouldn't it be better to specify in the documentation for the function that the functionality is there? My points in favor of adding it are the following:
- Direct portability from Godot, not losing any of the functionality
- Positive ranges are still usable, adding negative indices is only an extra, it doesn't take away functionality from positive ranges.
- Ranges allow this behavior, so it's not anti-Rust.
- In this case, changing a negative range to a positive range is not trivial, one would need to use
get_names_count()-i, which is not as simple as your usual len function.
For all of the above I'd advocate to keep the ranges in negative (using isize instead), but if not possible, I'd use either the Option<> one or add a new function named slice_from for the one with only start. I think both the Option and the slice from are more cumbersome than the range approach though. After your decision I'll change the code.
There was a problem hiding this comment.
I don't think this API is used nearly enough to deserve such a lengthy discussion 😅 but since we're here already...
In my opinion, consistency and principle of least surprise are important API design guidelines. There aren't any other places in godot-rust where negative ranges are used as wrap-around. If we allow these semantics here, people may be surprised why it doesn't work in other places that take ranges, e.g. substr or in the future index operators taking ranges.
There is also the precedent of Array::subarray_{deep|shallow} and Packed*Array::subarray() , which are all called slice in Godot, and currently take usize.
We might reconsider for them to take i64 to allow negative indices like here.
Btw, maybe this method should also be called subpath instead of slice, with a doc alias 🤔
There was a problem hiding this comment.
In my opinion, consistency and principle of least surprise are important API design guidelines.
Yes, absolutely, totally agree to this, no questions asked. I will follow precedent and if it's decided it should be changed, I'll change it myself if asked. My bad for not knowing the library better.
There aren't any other places in godot-rust where negative ranges are used as wrap-around.
Then, if it's ever considered, it shouldn't be that hard to change (the commit is here for easy copy paste)
There is also the precedent of Array::subarray_{deep|shallow} and Packed*Array::subarray() , which are all called slice in Godot, and currently take usize.
I would absolutely advocate for them taking isize instead. For the mentioned earlier, i64 is not intuitive for indices or slices, isize I feel makes more sense. It would provide feature parity with Godot too. For now, I'll do exactly as those methods and use usize and clamp the value to the length. No default value for end as it is not provided in these methods. To do it, I'd use the precedent from deep/shallow and make subpath and subpath_from, at most.
Btw, maybe this method should also be called subpath instead of slice, with a doc alias 🤔
Agreed, I don't know the API for the doc alias, so if you could point me to that, I'll use it.
There was a problem hiding this comment.
While isize can make more sense in theory, it's not a type that Godot uses, and thus none of our API has it (back to consistency 🙂 ).
Looking at generated classes, some of them with "index" semantics use i32:
- https://godot-rust.github.io/docs/gdext/master/godot/classes/struct.Node.html#method.get_index
- https://godot-rust.github.io/docs/gdext/master/godot/classes/struct.ArrayMesh.html
- https://godot-rust.github.io/docs/gdext/master/godot/classes/struct.SurfaceTool.html#method.add_index
- https://godot-rust.github.io/docs/gdext/master/godot/classes/native_menu/struct.ExAddIconCheckItem.html#method.index
- https://godot-rust.github.io/docs/gdext/master/godot/classes/text_edit/struct.ExGetLineWidth.html#method.wrap_index
A few use u32 🙈
This is something we need to live with. The Godot API is not perfect, and usize/isize aren't types that GDExtension uses. It's a bit hard to assume that conversions in the general case are always safe; a method taking i32 could in fact allow negative values, so we can't just assume usize fits and replace everywhere automatically. (Even though it would be nice... but it would take more effort)
But good that you brought this up, it shows that i64 is also very uncommon for index-like things. Overall we should reduce the number of types needed to a minimum. Since both isize and i64 are uncommon, let's stick to the ones with precedent:
usizefor pure indices/offsets that are guaranteed to be positive (e.g.Arrayhas those)i32for indices that can either be negative (wrap-around), or whose semantics aren't so clear-cut
Maybe the subarray* methods should also be changed to take i32? (Not in this PR, since that's a breaking change) 🤔
There was a problem hiding this comment.
Can't say I like the i32's choice, it feels as arbitrary as the i64, specially when using size for the unsigned and 32 for the signed. I stand on the side that using sizes for everything is the most intuitive, or at least, signed version should use double the bits (thus i64) since the signed part and the unsigned both refer to the same. That said, first is consistency and precedent is good to follow, so I understand the decision.
I agree that subarrays should use the signed version going forward for parity and usefulness.
Should I code the subpath already in i32 instead then? Just to confirm, I was going with usize to stick to subarray precedent
There was a problem hiding this comment.
I think i32 is OK if we want to support the negative wrap-around. We can then later adjust subarray to match this.
Agree that i32 isn't the best, but introducing isize would mean either of:
- having yet another type to which the user needs to convert (there's no reason why exactly this API should be different)
- if we stay consistent, change generated Godot APIs, which is an option but a bigger endeavor
There was a problem hiding this comment.
I think i32 is OK
Then I'll implement it in i32 using clamps for code consistency.
- having yet another type to which the user needs to convert (there's no reason why exactly this API should be different)
I agree that this is just bad and confusing, that's why I'd only choose this type if all "negative indexing" changed with it.
- if we stay consistent, change generated Godot APIs, which is an option but a bigger endeavor
My issue with even thinking of following Godot APIs is how they're either misleading at best (i64 when using u32 for hashes, for example) or error prone and wrong at best. They say that the end defaults to i32::MAX yet I believe the sizes of the path, subpath, and whatnot are not i32 but simply int. That's why so long as we do clamping (which is a code pattern in both subarray methods, for example) we should be safe in using what from a user standpoint makes the most sense from Rust (in the case of sizes, indexes and whatnot, usize and isize). But I acknowledge it is quite the endeavor + breaking change and this is just my perspective that it has changed from the start of the conversation to now, so, I don't have much ground to hold
|
I'll reply here to the overall comment I'll do as suggested and change it to usize, it makes sense, even if usize may be too big for the internal api. I would gladly implement an Iter, I think it would be nice, named get_names and get_subnames. I don't think the clutter here is a problem since the api for NodePath is small. I would suggest doing something else too, changing these implementations to get_name_checked and get_subname_checked, since I believe this implementation is useful, if you don't know the size of a path but it should have x at 2 place, for example, then get_name and get_subname would use the inner api. Tell me what you think though before working on this. And yeah! I'm adding the itests as soon as I have some spare time, sorry, this week is kinda hectic. |
|
The thing is, node path is in 95% of cases just used as "convert a string to it", most people don't even know they're using it because they're just working with So let's start with the simplest possible API: fn get_name(&self, idx: usize) -> StringName
fn get_name_count(&self) -> usize
fn get_subname(&self, idx: usize) -> StringName
fn get_subname_count(&self) -> usizeIf there's real demand, people will reach out for more. But it's also really easy for users to build their own abstractions on top of the above (either returning |
|
Gotcha, then I'd just use the inner implementation of get_name and get_subname, it's easier, and doing anything from Rust is not worth it imo given what's stated above. Another thing, what I commented on Discord regarding the one function I don't know what to do with:
I'm a bit confused as to whether this copies the value. If it does, then perfect, but if it doesn't, this means get_as_property_path returns either itself or a new NodePath, which is dangerous imo. |
It should be fine -- |
|
If you say it's fine, then I mark that box as checked, I had thought of having self instead of &self to make sure the user knows what's happening but, yeah, no need. Updated the tasks left, please do tell if I missed something. |
|
Any update after our discussion? Would be nice to merge it soon, to wrap up the builtins 🙂 |
Add desktop.ini to gitignore (for icons).
|
Since I haven't heard back in ten days, I completed the implementation. I ended up using the simplest possible API close to Godot itself. I additionally added While writing tests, I also discovered some weird upstream behavior of Thanks a lot for this pull request and the interesting discussion! 😊 |
TODO
#[itest]for NodePathoperator==andoperator!=Considerations
get_[sub]name_countfunctions return au32, because the ffi indicates ani64but a count is always positive, it comes from a sum ofsize, unless it overflows, in which case the functions from Godot side break.get_[sub]namefunctions return anOptioninstead of an emptyStringNameon failure, which is expected behavior in aRustcrate. It also takes au32as a parameter for the considerations above. It should not take ausizesince ausizecan be either 32 bits or 64 bits in the supported architectures of Godot.slicefunction takes a range since it makes more sense in a Rust crate. This range can have negative start or end, for the sake of feature parity with Godot's. To make inclusive ranges and unbounded ranges possible, it uses the default end value written in the Godot documentation.is_emptyandis_absoluteneed no changes from Rust's side, so they're called as are.get_concatenated_[sub]namesfunctions are taken as are because there's no need to eliminate the error messages even though they make little sense, a path that contains nothing should just return an empty string when it's joined. They can be useful if someone wants them, though, so erasing them for the sake of erasing them does not make much sense either. If this is thought otherwise, I'll reimplement them myself.