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

Rework Gd constructors, add default() to engine classes #486

Merged
merged 2 commits into from
Nov 19, 2023

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Nov 15, 2023

Changes:

  • Rename new_default -> default
  • Rename new -> from_object
  • Rename with_base -> from_init_fn
  • Extend impl Default from user types to engine types that have a new() constructor (i.e. ref-counted ones)
  • Rename GodotInit -> GodotDefault to reflect new meaning
  • Enable Gd::default() and Gd::new_alloc() constructors depending on memory strategy

Old names remain deprecated where possible.

Add extension methods T::new_gd() and T::alloc_gd() as shortcut for Gd::<T>::default() and Gd::<T>::new_alloc().
These are provided via extension trait UserClass (part of prelude) and likely almost always the preferred way of constructing.


New API overview:

Type \ Memory Strategy Ref-counted Manually managed Singleton
Engine type Resource::new() Node::new_alloc() Os::singleton()
User type MyClass::new_gd() MyClass::alloc_gd() (not yet implemented)
Gd (rarely used constructors) Gd::<T>::default() (not available) (not available)
Other Gd<T> constructors Usage
from existing Rust object Gd::<T>::from_object(obj)
from closure accepting base Gd::<T>::from_init_fn(closure)
from instance ID Gd::<T>::from_instance_id(id)
from instance ID (fallible) Gd::<T>::try_from_instance_id(id)

As you see, the naming is not very consistent, unfortunately. There are some constraints:

  1. I cannot use MyClass::new(), since it's very likely that a user already provides such a constructor.
  2. I could use Resource::new_gd() to have new_gd() everywhere, but it would mean all engine types are now harder to construct. Unlike with user types, there is no other new or so that could collide.
  3. I could use Gd::new() to mean Gd::default(), but:
    • This constructor is very rarely useful, since we now have extensions on the inner types that are more concise, making new not a great choice of name (as this indicates the most commonly used one).
    • I was actually considering from_init_fn() to be named new() just because of how useful it is, and since it's a bit hard to discover otherwise. Lots of people didn't know you could create custom Gd<T> instances where T would receive a base.

This may need a bit more thought, feedback is welcome.

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Nov 15, 2023
@GodotRust
Copy link

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

@StatisMike
Copy link
Contributor

StatisMike commented Nov 15, 2023

@Bromeon So, if I understand correctly, to use Gd::<T>::from_init_fn(closure) you could call use closure similar to init method?

I could use Resource::new_gd() to have new_gd() everywhere, but it would mean all engine types are now harder to construct. Unlike with user types, there is no other new or so that could collide.

You mean typying _gd as being more hard, or are there some strings attached with new_gd() there?

I myself find new_gd() very informative and intuitive. IMO new() make it a little less explicit that you won't be receiving Resource, but Gd<Resource>, which is obvious with new_gd. I would vote on giving new_gd() for both UserDomain and EngineDomain classes but I feel that different constructors make the difference between UserDoman and EngineDomain classes more explicit.

I was actually considering from_init_fn() to be named new() just because of how useful it is, and since it's a bit hard to discover otherwise

I feel like from_* methods are great, and this convention should be used. On the other hand, Gd::<T>::new() makes more sense to me than T::new(), tbh (as above) - but less than Gd::<T>::from_init_func(). Per the discoverability point: it can be remedied by making a chapter on Gd constructors with examples.

@Bromeon
Copy link
Member Author

Bromeon commented Nov 15, 2023

So, if I understand correctly, to use Gd::<T>::from_init_fn(closure) you could call use closure similar to init method?

Just like with_base() now:

let obj = Gd::from_init_fn(|base| {
    // accepts the base and returns a constructed object containing it
    MyClass { base, other_field: 732 }
});

You mean typying _gd as being more hard, or are there some strings attached with new_gd() there?

I myself find new_gd() very informative and intuitive. IMO new() make it a little less explicit that you won't be receiving Resource, but Gd<Resource>, which is obvious with new_gd. I would vote on giving new_gd() for both UserDomain and EngineDomain classes but I feel that different constructors make the difference between UserDoman and EngineDomain classes more explicit.

Thanks for the input! The typing is not really a huge deal, but more that there's not really a way to create engine types without putting them in a Gd<T>. So it's somewhat redundant, considering engine classes in isolation. A name different from new also makes it look like there could be another "primary" constructor called new (although this is already a problem with new_alloc).

I was also thinking about short synonyms like "create", "make", etc. but they are less discoverable and might collide with other Godot methods.

Also, how to name new_alloc() in that case? new_alloc_gd is very long... or just alloc_gd, but that's again a bit harder to identify as a constructor.


Per the discoverability point: it can be remedied by making a chapter on Gd constructors with examples.

Yep, that's definitely planned, possibly as part of the cheatsheet.

@StatisMike
Copy link
Contributor

but more that there's not really a way to create engine types without putting them in a Gd. So it's somewhat redundant, considering engine classes in isolation.

Exactly - you can't create a Resource, just Gd<Resource>. new_gd() makes it explicit. There is no new method, because there can't be.

IMO it could be looked as a shortcut for Gd::<Resource>::new() (at least theorhetically, as you are planning to rename it, damn 😜).

I was also thinking about short synonyms like "create", "make", etc. but they are less discoverable and might collide with other Godot methods.

Full agreement

Also, how to name new_alloc() in that case? new_alloc_gd is very long... or just alloc_gd, but that's again a bit harder to identify as a constructor.

Yeah, that's tricky. I prefer new_alloc() myself. I feel that it emphasise that you are allocating this obj in Godot memory and you are responsible for freeing it.

I fully understand that this may seem a little inconsequential as new_gd() returns Gd, the same as new_alloc(), but I don't have an idea how this could be handled. new_* seems to me like a must. They need to began the same and be different, so IDE can show which one is on this class and if the user need to manage memory manually. They need to be also different enough to be noticeable at first glance.

@Bromeon
Copy link
Member Author

Bromeon commented Nov 15, 2023

IMO it could be looked as a shortcut for Gd::<Resource>::new() (at least theorhetically, as you are planning to rename it, damn 😜).

About that, it's possible but not that useful, see my previous explanation:

  1. I could use Gd::new() to mean Gd::default(), but:
  • This constructor is very rarely useful, since we now have extensions on the inner types that are more concise, making new not a great choice of name (as this indicates the most commonly used one).
  • I was actually considering from_init_fn() to be named new() just because of how useful it is, and since it's a bit hard to discover otherwise. Lots of people didn't know you could create custom Gd<T> instances where T would receive a base.

new_* seems to me like a must. They need to began the same and be different, so IDE can show which one is on this class and if the user need to manage memory manually.

Yeah, I'm getting that feeling too. It's not always obvious whether a type is ref-counted or not, so a user starting to type SomeType::new... should be guided by IDE to complete a method.

Changes:
- Rename `new` -> `from_object`
- Rename `new_default` -> `default`
- Rename `with_base` -> `from_init_fn`
- Extend `impl Default` from user types to engine types that have a new() constructor (i.e. ref-counted ones)
- Rename `GodotInit` -> `GodotDefault` to reflect new meaning
- Enable `Gd::default()` and `Gd::new_alloc()` constructors depending on memory strategy

Old names remain deprecated where possible.

Add extension methods `T::new_gd()` and `T::alloc_gd()` as shortcuts for creating Gd<T> instances
There is almost no use case for this, since this is covered by `UserT::alloc_gd()` and `Engine::new_alloc()`
in a shorter form. If we need generic programming, we should design it as a whole.
@Bromeon Bromeon added this pull request to the merge queue Nov 19, 2023
Merged via the queue into master with commit 8ec6a22 Nov 19, 2023
15 checks passed
@Bromeon Bromeon deleted the qol/gd-constructors branch November 19, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants