Skip to content
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

AsObjectArg trait enabling implicit conversions for object parameters #800

Merged
merged 6 commits into from
Jul 22, 2024

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Jul 15, 2024

Adds a trait AsObjectArg<T>.
Godot engine functions that have a parameter impl AsObjectArg<T> will now accept:

  • Gd<U> where U: Inherits<T>
  • &Gd<U>
  • Option<Gd<U>>
  • Option<&Gd<U>>
  • Gd::null_arg(), a special expression only valid for null arguments

Closes #156.
Closes #796.

Needs to land in v0.2 because some call-site patterns like node.clone().upcast() are now ambiguous and no longer possible.


Older message

To be decided:

  1. Should we impl AsArg<T> for Gd<T> (i.e. values, not just references)?
    • allows destructive move
    • creates possible move errors that need a lot of fine-tuning, instead of just always passing &obj
    • can create more monomorphizations (especially if also both Option<Gd<T>> and Option<&Gd<T>> are supported)
  2. What is a good inner type?
    • RawGd creates lots of copies, and may be unsafe/unsound to cast, plus needs destructor deactivated
    • raw object pointers that can be null?
    • dedicated ArgView type wrapping a pointer?

@Bromeon Bromeon added feature Adds functionality to the library c: engine Godot classes (nodes, resources, ...) breaking-change Requires SemVer bump labels Jul 15, 2024
@Ughuuu
Copy link
Contributor

Ughuuu commented Jul 15, 2024

Can't comment much as I don't really understand what this will achieve. With this I see you are not calling explicitly .clone anymore in some cases. Does it still clone in those cases or not anymore?

@Bromeon
Copy link
Member Author

Bromeon commented Jul 15, 2024

Does it still clone in those cases or not anymore?

Possibly in the initial implementation, but my plan is definitely to get rid of any unnecessary clones. Since this would then be an internal perf improvement, it might be a separate PR.

@lilizoey
Copy link
Member

I think we could just use Option<&__GdextObject>? (__GdextObject is the opaque type we generate for use in GDExtensionObjectPtr etc.) This would effectively be a nullable pointer with a lifetime. So we can more easily avoid use-after-free and similar. While keeping the cheapness of passing around a pointer. Ideally this would use extern types but that's not been implemented for a while now.

This would probably force the trait to be an unsafe trait though, since it'd effectively erase the type. But it should also simplify implementation a fair bit.

@Bromeon
Copy link
Member Author

Bromeon commented Jul 16, 2024

The problem with Option<&__GdextObject> (or any new type wrapping a pointer) is that arguments are passed in to the signature tuple, so we'd need to implement again all the traits that RawGd has: ToGodot/GodotConvert/GodotFfi/GodotNullableFfi/GodotFfiVariant/GodotType/...

I might see how bad it is, maybe we can reuse code somehow.

@Bromeon Bromeon force-pushed the feature/param-conversions branch 4 times, most recently from fa413fa to 475fc0d Compare July 17, 2024 15:19
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-800

@godot-rust godot-rust deleted a comment from GodotRust Jul 17, 2024
@Bromeon Bromeon changed the title AsArg trait enabling implicit conversions for object parameters (WIP) AsObjectArg trait enabling implicit conversions for object parameters Jul 20, 2024
@Bromeon Bromeon force-pushed the feature/param-conversions branch 3 times, most recently from 56f6387 to f1b25a5 Compare July 20, 2024 12:46
@Bromeon Bromeon marked this pull request as ready for review July 20, 2024 12:54
@Bromeon Bromeon added this to the 0.2 milestone Jul 21, 2024
@Bromeon Bromeon added this pull request to the merge queue Jul 22, 2024
Merged via the queue into master with commit dc82c84 Jul 22, 2024
15 checks passed
@Bromeon Bromeon deleted the feature/param-conversions branch July 22, 2024 19:37
@Bromeon Bromeon added the bug label Jul 27, 2024
@Bromeon
Copy link
Member Author

Bromeon commented Jul 27, 2024

Also declared bugfix, since it fixes #156 which wasn't previously possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Requires SemVer bump bug c: engine Godot classes (nodes, resources, ...) feature Adds functionality to the library
Projects
None yet
4 participants