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

Default func params from Godot #380

Closed

Conversation

you-win
Copy link
Contributor

@you-win you-win commented Aug 13, 2023

Heavily WIP and might not even be desirable.

Adds the ability to omit parameters when calling Rust methods from GDScript and also have default parameters show up in the GDScript bindings.

This does not actually pass any default values from GDScript. The responsibility is still on the developer to provide their own default values.

Notable changes

  • pass arg_count when using varcall. The current implementation aims for tuples to still be used instead of a Vec for params
  • varcall now requires parameters to implement Default
    • This requirement is awful. I think the ideal would be requiring something that is either a T or Option<T>, but I'm not sure if that's possible. This requirement also means that the itests are currently broken
    • Maybe separate trait impls might solve this?

TODO

  • fix itests
  • implement FromVariant for Option<T>. Needed since things like Option<i32> are not valid. Could also just be an impl just for primitive Godot types
  • remove requirement on Default for impl_varcall_signature_for_tuple?
  • somehow bind default values from the #[func(defaults = [...]] macro to varcalls?

…count, update default func param tests to test happy path first
@GodotRust
Copy link

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

@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Aug 13, 2023
@lilizoey
Copy link
Member

lilizoey commented Aug 13, 2023

I think it may be better to change the signatures to look something like:

(R, (P1, P2, ...), (D1, D2, ...))

Where P* are required arguments and D* are optional. Or maybe we even make our own ZST to be able to name these a bit better, like

struct Signature<
  RETURN_TYPE,
  REQUIRED_ARGS,
  DEFAULT_ARGS,
  const HAS_VARARGS: bool
> {
  ...
}

That way we can distinguish between a function like fn foo(a: i32) and fn foo(a = 5: i32) (not real syntax). And we can also only require that the default arguments implement default, instead of all of them, and it makes it easy to add in like vararg-lists if we want to support that too.

(This would likely also involve changing the traits to have new type aliases for the default arguments)

@Bromeon
Copy link
Member

Bromeon commented Aug 27, 2023

@you-win are you still interested in pursuing this? I have to refactor the proc-macros file structure a bit, so there might be some merge conflicts upcoming...

@you-win
Copy link
Contributor Author

you-win commented Aug 27, 2023

I can rebase my PR based off of the refactor, no worries.

Edit: If someone wants to salvage this PR, please feel free to do so. I'm admittedly busy with other things

@Bromeon
Copy link
Member

Bromeon commented Sep 24, 2023

Any update on this one? The more we wait, the more the code will diverge and introduce conflicts 🙈
If you don't have time for now, I can give it another attempt, but only in a few weeks.

@Bromeon
Copy link
Member

Bromeon commented Oct 14, 2023

Since there hasn't been an update since August and the PR had significantly diverged from master, I'm going to close it. Nevertheless, thanks a lot! It seems that default parameters need a bit more thought, so any experiment in that space is helpful. We'll very likely revisit this topic again later 🙂

@Bromeon Bromeon closed this Oct 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants