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

arrow2-convert migration 4: support serde-based types in codegen #3902

Merged
merged 9 commits into from
Oct 19, 2023

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Oct 17, 2023

This introduces the rust.attr.serde_type attribute, allowing you to use any serde-compatible Rust types in our IDL.

table ViewportLayout (
  "attr.rust.derive": "PartialEq",
  "attr.rust.override_crate": "re_viewport"
) {
  space_view_keys: [ubyte] (order: 100, "attr.rust.serde_type": "std::collections::BTreeSet<re_viewer_context::SpaceViewId>");

  tree: [ubyte] (order: 101, "attr.rust.serde_type": "egui_tiles::Tree<re_viewer_context::SpaceViewId>");

  auto_layout: bool (order: 102);
}

This unblocks further blueprint experimentations, and is the last blocker to sunset arrow2-convert.

  • SpaceViewComponent, SpaceViewMaximized & ViewportLayout are now all implemented that way.
  • re_viewport is now free of arrow2-convert.

arrow2-convert migration PR series:

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 demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added 🟦 blueprint The data that defines our UI codegen/idl include in changelog do-not-merge Do not merge this PR labels Oct 17, 2023
@teh-cmc teh-cmc force-pushed the cmc/arrow2convert_be_gone_3_for_real branch from 507b862 to bb4da17 Compare October 18, 2023 06:43
@teh-cmc teh-cmc force-pushed the cmc/arrow2convert_be_gone_4 branch from 53700bf to d821641 Compare October 18, 2023 06:45
@teh-cmc teh-cmc force-pushed the cmc/arrow2convert_be_gone_4 branch from d821641 to 0b54344 Compare October 18, 2023 09:18
Base automatically changed from cmc/arrow2convert_be_gone_3_for_real to main October 18, 2023 13:46
teh-cmc added a commit that referenced this pull request Oct 18, 2023
… out dependencies (#3897)

This introduces the `rust.attr.override_crate` attribute, which is
necessary for the serde-codegen story.

This attribute also allows us to generate some fundamental types such as
`InstanceKey` directly into `re_types_core` rather than `re_types`,
making it possible to reduce the number of crates that depend on the
giant `re_types`.

- The `AutoSpaceViews` & `PanelView` components are now back into their
respective home (`re_viewport` & `re_viewer`, respectively).
They were temporarily moved it because we had no support for
`custom_crate`.
They will be joined by their more complicated siblings in the next PR,
which implements the necessary serde-codegen support.

- `InstanceKey`, `Clear` and `ClearIsRecursive` are now generated into
`re_types_core`.
This allows `re_query`, `re_arrow_store` and `re_data_store` to drop
their dependency on `re_types`.

- Generated code now picks up arrow from `re_types_core::external`
instead of importing it directly.
This means crates hosting generated code are not forced into import
`arrow2` directly.

---


`arrow2-convert` migration PR series:
- #3853 
- #3855
- #3897
- #3902
@teh-cmc teh-cmc force-pushed the cmc/arrow2convert_be_gone_4 branch from 0b54344 to ad9c851 Compare October 18, 2023 13:48
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Oct 18, 2023
@emilk emilk self-requested a review October 18, 2023 14:08
crates/re_types/src/datatypes/tensor_buffer.rs Outdated Show resolved Hide resolved
crates/re_types_builder/src/codegen/rust/serializer.rs Outdated Show resolved Hide resolved
)
.boxed()
})
}
Copy link
Member

Choose a reason for hiding this comment

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

The generated code is not very readable :/ is there some low-hanging fruit that can help?

Maybe:

  • Some more use statements at the top
  • More let bindings
  • Add some comments

Copy link
Member Author

Choose a reason for hiding this comment

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

Did some include clean up. Everything else will have to be refactored pretty soon to drop arrow2 anyway.

crates/re_viewport/src/blueprint/space_view_component.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/blueprint/space_view_component.rs Outdated Show resolved Hide resolved
@teh-cmc teh-cmc merged commit e537874 into main Oct 19, 2023
@teh-cmc teh-cmc deleted the cmc/arrow2convert_be_gone_4 branch October 19, 2023 07:39
teh-cmc added a commit that referenced this pull request Oct 19, 2023
The end of our wonderful journey.

- `NumInstances` control column now has an actual dedicated component
type.
- `EntityPath` is now a component.
- `Into<Cow<Self>>` impls have been cleaned up to generate way less
code.
- `arrow2_convert` is fully gone.


---

`arrow2-convert` migration PR series:
- #3853 
- #3855
- #3897
- #3902
- #3917
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants