-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
Tracking issue for builtin feature-completeness #310
Comments
I've been checking the vectors and these is what I've found so far:
|
im working on |
316: Reimplement `slerp` on Vector3 r=Bromeon a=T4rmin Reimplemented the `slerp` method on Vector3 with the addition of documentation and several tests on it. Addresses part of #310 to (possibly?) complete Vector3 methods (leaving the lacking documentations aside). A notable change from the original Godot calculations is just directly using `.normalized()` on `axis` for `unit_axis` than manually doing it by `axis / axis_length_sq.sqrt()` on line 314. Co-authored-by: Aaron Lawrence <[email protected]>
I've gotten a bit side-tracked. Feel free to have a look here if someone else wants to continue, https://github.com/lilizoey/gdextension/tree/feature/stringy-types. Though there are some things i'm not sure if should go into a PR as is (like the indexing operators). |
Dictionary
StatusThe only functions that aren't present are The only different function is Remaining WorkIf we believe it is worthwhile to copy the |
Array
StatusLike dictionary there is no The "type" methods don't make sense in the context of
Remaining Work
I personally don't see value in I don't know about the edit: |
PackedByteArray
StatusThis is a tough one. It could be far more complete than described here. For example the There is also minor strangeness. Remaining workI don't know for sure, but it looks like lots. |
PackedArrayOutside of packed byte array every godot PackedArray has the same functions and every rust PackedArray has the same methods. Because of that we can generalize here:
StatusThis is mostly if not entirely complete. Remaining workvalidate |
Status
StatusSpherical_cubic functions need to be implemented. Remaining workCalled out in status |
That would be |
Working on the It also relies on these hand rolled cubic interpolation functions. Is there an appetite for using a crate to supply cubic interpolation functions, or is the preference maintaining minimal dependencies? |
In the past, we may have been a bit overzealous at porting Godot functionality to Rust, in an attempt to improve performance. There was also a (only partially correct) understanding that FFI calls would be inherently slow and to be avoided at all costs. However, RIIR (rewrite it in Rust) doesn't come for free: effort for testing and maintenance, risk of discrepancies in behavior, added compile-time overhead. If Godot fixes something, we are either left with different behavior or need to follow suit. Since these re-implementations were mostly done without measuring, it remains to be seen whether they're actually worth it. For simpler things like vector operations that are delegated to
Ideally identical, or at least very similar outcomes. That may already not be the case for some of the geometric types we built -- which can lead to subtle bugs in larger programs 🤔 but it's hard to say in general. If an algorithm is 10x faster than another while retaining 99.7% accuracy, then that's likely the better choice.
I'm generally quite hesitant to add new dependencies, because they have an impact not only on users, but also our CI workflows, slowing down contributions. For something where the added value is unclear, even more so. And massive crates with type shenanigans ( Yet, as mentioned above, I also don't want to make people do silly code-porting exercises and significantly increase the maintenance surface of the library, when there's no clear need. Using Godot's impl until it causes actual problems is a reasonable approach in my view. TLDR: I think the first step would be to prove that RIIR is bringing real-world benefits, or else focus on other tasks. We already have a massive library at hand that has parity with Godot and needs zero maintenance and zero compile time on our side, it's called Godot 😉 |
Sounds good. Given that it probably makes most sense to get 100% builtin parity before considering optimizations. Per |
As mentioned, the RIIR may also have been a mistake in some cases. If we run into problems due to it, or are faced with increased maintenance burden, I'm happy to yeet that code and revert back to Godot's implementation. |
#310 - Array builtin completeness
#310 Quaternion Function Completeness
Vector3 should be green because #316 implemented slerp |
Thanks, updated according to your last two comments. There are a few entries that mention documentation. In cases where the Rust API is the same as Godot's, we can reuse the provided docs from Godot. I created #584 for it. |
Is there an argument for implementing I guess there's the difference between returning |
We'd need to double-check that the current implementation of If that is the case, |
More detailed breakdown of the builtin types than present in #24.
Similar to #209 and #143, there are more builtins missing functions, here's a list of our progress when it comes to implementing all functionality.
The missing functionality here is largely going to be easy to reimplement, unless otherwise specified. We should try to reimplement what we can in rust, but for many types (like for instance the stringy types) it's likely not going to be very easy to do so. In those cases we can simply make a wrapper around a call to the
InnerX
type, (for instanceInnerGString
forGString
). Which simply makes a call to godot to call that function.For anything labelled ❔ it would be good to check if all the godot methods are implemented yet. Feel free to leave a comment on this post if you do. Keep in mind that not everything is directly implemented as a method in rust, some things for instance are trait implementations, for instance
Projection(Transform3D from)
was reimplemented as aimpl From<Transform3D> for Projection { .. }
.Legend
🟥 bare-bones, missing almost everything
🟡 most functions re-implemented
💚 all planned functions re-implemented, (could still mean there are QOL things we could add)
🚧 work already in progress
❔ to be confirmed if everything is re-implemented
Stringy Types
GString
💚 Tracking issue for builtin feature-completeness #310 (comment)StringName
💚 Tracking issue for builtin feature-completeness #310 (comment)NodePath
💚Bounding Boxes
Aabb
💚 Reimplemented Aabb.rs in godot-core/src/builtin #280Rect2
💚 Reimplement Rect2 functions #242Rect2i
💚Matrices
Basis
💚 Implement the matrix types #124Transform2D
💚 Implement the matrix types #124Transform3D
💚 Implement the matrix types #124Projection
💚 Reimplements ManyProjection
Methods in Rust #179fovy_of
is currently a wrapper aroundInnerProjection::get_fovy
create_fit_aabb
, initially not implemented becauseAabb
wasn't implementedVectors
Vector2
💚slerp
with gdnativeVector2i
💚Vector3
💚slerp
on Vector3 #316Vector3i
💚Vector4
💚Vector4i
💚snapped
for int vectors 🟥Misc Math
Plane
💚 godot-core: builtin: reimplement Plane functions/methods #268Quaternion
💚 PR #4/5 Astolfo feature/builtin-quaternion #68spherical_cubic_interpolate
spherical_cubic_interpolate_in_time
Arrays
Packed*Array
💚to_byte_array()
len()
should have a doc-alias forsize()
Add doc-aliases to functions that have their name changed from godot #232FromIterator
impl for&T
for the copy-arraysPackedByteArray
🟡compress/decompress
should likely just defer to godot implementationencode_*/decode_*
andget_string_from_*
can be done throughslice()
, is this sufficient?has_encoded_var
hex_encode
Standard Collections
Dictionary
🟡is_read_only()
andmake_read_only()
Array
🟡bsearch_custom()
sort_custom()
is_read_only()
andmake_read_only()
Other
Rid
💚 Implement support for RID #171Callable
💚 Add implementation of Callable #231, the unimplemented functionality is currently broken and cannot be used in gdextensionSignal
💚, not going to be easy to reimplementThe text was updated successfully, but these errors were encountered: