Skip to content

Replace MarkerShape with code-generated enum type#5336

Merged
emilk merged 29 commits intomainfrom
emilk/codegen-enums-for-real
Feb 29, 2024
Merged

Replace MarkerShape with code-generated enum type#5336
emilk merged 29 commits intomainfrom
emilk/codegen-enums-for-real

Conversation

@emilk
Copy link
Copy Markdown
Member

@emilk emilk commented Feb 28, 2024

What

Also replaces Corner2D and ContainerKind, but those are blueprint only and thus not user facing (yet).

The user-facing code should stay mostly the same.

I also improved the generated documentation for all components and datatypes

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@emilk emilk added codegen/idl include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages labels Feb 28, 2024
/// The top-level description of the Viewport.
table ContainerBlueprint (
"attr.rerun.scope": "blueprint",
"attr.rust.derive": "Default",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The old code had a bug, which defaulted the container_kind to the invalid value 0

@emilk emilk marked this pull request as ready for review February 28, 2024 21:08
@teh-cmc teh-cmc self-requested a review February 29, 2024 07:55
@teh-cmc
Copy link
Copy Markdown
Contributor

teh-cmc commented Feb 29, 2024

You shouldn't need an extra attribute to find the associated enum type.

field.type_().index() should give you the global index of the enum type associated with a given field, which you can then use to inspect it from the global enum registry.

E.g.:

diff --git a/crates/re_types_builder/src/objects.rs b/crates/re_types_builder/src/objects.rs
index 07d1f0fb38..e1d32f5516 100644
--- a/crates/re_types_builder/src/objects.rs
+++ b/crates/re_types_builder/src/objects.rs
@@ -795,6 +795,7 @@ impl ObjectField {
 
         let attrs = Attributes::from_raw_attrs(field.attributes());
 
+        dbg!(enums[field.type_().index() as usize]);
         let typ = if let Some(enum_type) = attrs.try_get(&fqname, crate::ATTR_ENUM_TYPE) {
             // Hack needed because flattbuffers report fields of enum types as integers.
             Type::Object(enum_type)

Copy link
Copy Markdown
Contributor

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

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

Looks great except for that superfluous enum_type attribute and the miscellaneous hacks that come with it.

"attr.rust.derive": "PartialEq, Eq, PartialOrd, Copy",
"attr.rust.repr": "transparent"
enum MarkerShape: byte (
"attr.docs.unreleased"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why unreleased?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@emilk emilk requested a review from teh-cmc February 29, 2024 11:08
@emilk emilk merged commit 588eb3c into main Feb 29, 2024
@emilk emilk deleted the emilk/codegen-enums-for-real branch February 29, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codegen/idl include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use enum in codegen to specify constant-constants

2 participants