Skip to content

Conversation

fpdotmonkey
Copy link
Contributor

A default value for Rid makes it possible to use it as a member of GodotClass-derived struct, which can be useful for custom Resources.

The default value of Invalid is both logical--it shouldn't point to any resource a user doesn't specify--and it's what's done in-engine (godot@46dc27/core/templates/rid.h:40 - uint64_t _id = 0;).

A default value for `Rid` makes it possible to use it as a member of
`GodotClass`-derived struct, which can be useful for custom Resources.

The default value of `Invalid` is both logical--it shouldn't point to
any resource a user doesn't specify--and it's what's done in-engine
(godot@46dc27/core/templates/rid.h:40 - `uint64_t _id = 0;`).
@GodotRust
Copy link

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

@Bromeon
Copy link
Member

Bromeon commented Feb 22, 2024

You should have waited for the response on Discord 😉

I'm not a huge fan of adding Default for types whose default representation is invalid; rationale see here.


A default value for Rid makes it possible to use it as a member of GodotClass-derived struct, which can be useful for custom Resources.

You can achieve this with #[init(default = Rid::Invalid)] or an overridden init() method -- both are more explicit.


Btw, the API of Rid might become more like InstanceId in the future.
Its current definition

pub enum Rid {
    Valid(NonZero<u64>),
    Invalid,
}

is not great, because Rid::Valid is not its own type, so you can't express that you have a valid Rid and need to defensively check is_valid()/is_invalid() in the code...

If we had something like type Rid = Option<ValidRid>, then you could simply reuse all the outer APIs, including Option::default(). This might need a bit more discussion, since Callable and Signal are very similar, and maybe even some geometric types like Plane.

@fpdotmonkey
Copy link
Contributor Author

This seems reasonable. I'll close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants