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

Bitsets implement Default #162

Merged
merged 1 commit into from
Mar 14, 2023
Merged

Conversation

mio991
Copy link
Contributor

@mio991 mio991 commented Mar 9, 2023

Adds a Default implementation for Enums.

This came about because I wanted to use ArrayMesh::add_surface_from_arrays with SurfaceTool::commit_to_arrays but had no idea which ArrayFormat to use. In the docs the default value is 0.

@mio991 mio991 force-pushed the enum_impl_default branch from f5f49cd to 2772e08 Compare March 9, 2023 11:14
@Bromeon
Copy link
Member

Bromeon commented Mar 9, 2023

Thanks! This was already suggested in #101 (comment), but there are a few problems with a Default implementation for enums:

  1. The value 0 does not necessarily represent a valid enumerator.
    • Example FileAccess::ModeFlags doesn't have one with value 0.
  2. If 0 is valid, it may not be the most "obvious" choice for a default. Enumerators may be ordered systematically, not by usage frequency.
    • Example: GDExtension::InitializationLevel has CORE as 0, but typically used is SCENE.
  3. 0 assigned to multiple enumerators -- this will play a role when using the string repr.
    • Example: CameraServer::FeedImage has FEED_RGBA_IMAGE, FEED_YCBCR_IMAGE, FEED_Y_IMAGE with value 0

It also seems like it wouldn't solve the problem you have here.

This came about because I wanted to use ArrayMesh::add_surface_from_arrays with SurfaceTool::commit_to_arrays but had no idea which ArrayFormat to use. In the docs the default value is 0.

Enum::default() may not be the same value as the default parameter's value.

What you're looking for here is an implementation of default parameters rather than enum defaults. There has been some discussion over at gdnative: godot-rust/gdnative#814

@Bromeon Bromeon added feature Adds functionality to the library c: engine Godot classes (nodes, resources, ...) labels Mar 9, 2023
@mio991 mio991 force-pushed the enum_impl_default branch from 2772e08 to 5965e0b Compare March 11, 2023 13:26
@mio991
Copy link
Contributor Author

mio991 commented Mar 11, 2023

Changed it so that only Bitsets implement Default.

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! For now, I think default() is a good enabler, although in the future I might consider adding something like an ALL_UNSET constant or so instead.

Just a small comment, then should be good to go 🙂

bors try

derives.push("Default");
}

let derives: Vec<_> = derives.into_iter().map(ident).collect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

collect() should not be necessary, I think the expression inside quote! can also work on an iterator.

bors bot added a commit that referenced this pull request Mar 12, 2023
@bors
Copy link
Contributor

bors bot commented Mar 12, 2023

try

Build succeeded:

@mio991 mio991 force-pushed the enum_impl_default branch from 5965e0b to 62f0f8b Compare March 14, 2023 16:10
@Bromeon
Copy link
Member

Bromeon commented Mar 14, 2023

Thanks!
bors r+

@Bromeon Bromeon changed the title Enums implement Default Bitsets implement Default Mar 14, 2023
@bors
Copy link
Contributor

bors bot commented Mar 14, 2023

Build succeeded:

@bors bors bot merged commit b0762e9 into godot-rust:master Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants