Skip to content

Conversation

beicause
Copy link
Contributor

@beicause beicause commented Jun 24, 2025

Can't define a field of [Rid; N] in struct with #[class(init)] because Rid doesn't implement Default trait currently. Rid should also implement Default like other built-in types.

@Yarwin
Copy link
Contributor

Yarwin commented Jun 24, 2025

I'm sorry, but our stance haven't changed since #623. Use #[init( val = [Rid::Invalid; 6])] instead.

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.

@Yarwin Yarwin closed this Jun 24, 2025
@beicause beicause deleted the impl-rid-default branch June 24, 2025 14:17
@beicause
Copy link
Contributor Author

Thank you for your reply. However, I hold an opposing view on this. The Rid type can be either valid or invalid, just like Rust's Option type, which defaults to None.

If we designed it differently - where Rid is always NonZeroU64 and Option<Rid> represents "may be invalid" - then we wouldn't need the Default trait.

@Bromeon
Copy link
Member

Bromeon commented Jun 25, 2025

If we designed it differently - where Rid is always NonZeroU64 and Option<Rid> represents "may be invalid" - then we wouldn't need the Default trait.

The design may still change, see discussion in #171. In fact my idea was to make it closer to InstanceId, but apparently there were technical limitations of trait impls on Option<Rid>.

Suggestions in that direction are welcome! 👍

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components status: wontfix This will not be worked on labels Jun 25, 2025
@Bromeon
Copy link
Member

Bromeon commented Jun 25, 2025

To elaborate a bit, the current design has two problems:

  1. There isn't a type to express "occupied RID". APIs that return RID may return 0 or not, but it's not clear. Users have to use match/if let to access the number, even if the creator can guarantee the validity.

    • Option<Rid> is one approach, if it's reconcilable with traits.
    • enum with a dedicated OccupiedRid type is an alternative.
  2. RIDs are untyped. However, in Godot they're usually specific to a certain kind of resource (especially for servers like physics, rendering, etc).

    • This might even be a soundness issue, if RIDs are passed to the wrong resource and interpreted without checks.
    • Concrete types like PhysicsRid would address it. We could still have UntypedRid/RawRid with some unsafe operations.

Depending on the new design, we can then reconsider whether Default is needed and useful (or auto-provided).

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

Labels

c: core Core components feature Adds functionality to the library status: wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants