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

Use enum in codegen to specify constant-constants #3384

Closed
Tracked by #4813
jleibs opened this issue Sep 20, 2023 · 3 comments · Fixed by #5336
Closed
Tracked by #4813

Use enum in codegen to specify constant-constants #3384

jleibs opened this issue Sep 20, 2023 · 3 comments · Fixed by #5336
Assignees
Labels
Milestone

Comments

@jleibs
Copy link
Member

jleibs commented Sep 20, 2023

Our enums currently get converted into classes that implement arrow unions, which I don't believe is correct. The more common pattern of using enums in fbs files, for example:

enum ViewDir: byte {
    Up = 1,
    Down = 2,
    Right = 3,
    Left = 4,
    Forward = 5,
    Back = 6,
}

Should map to an appropriate enum of constants in each language.

@Wumpf
Copy link
Member

Wumpf commented Sep 20, 2023

would also be useful for log level strings!

@abey79
Copy link
Member

abey79 commented Dec 18, 2023

...and for blueprint container kind.

@emilk
Copy link
Member

emilk commented Jan 11, 2024

There are two ways we can encode the enums: using names ("Up", "Down", …), or using integers (1, 2, …).

I think using the names is superior, as it means the arrow data is self-descriptive.
This means viewing them in e.g. the table view would show the variant name, instead of just an integer.
However, it means renaming an enum variant is a breaking change (just like for the name of a struct field).

I believe the best way to encode these in Arrow is by using a Sparse Union with the NULL type for each field. This means only 8 bits should be required for each value.

@emilk emilk added the 🏹 arrow concerning arrow label Jan 11, 2024
@emilk emilk self-assigned this Jan 11, 2024
@emilk emilk removed their assignment Jan 11, 2024
emilk added a commit that referenced this issue Jan 12, 2024
### What
* Part of #3384

Small restructure and refactor to improve error reporting in the
codegen, especially when it comes to `enum` being unimplemented.

See commit messages.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4786/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4786/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4786/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4786)
- [Docs
preview](https://rerun.io/preview/3e8e9722b7d64b28ca55453152ab80d259600cfe/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/3e8e9722b7d64b28ca55453152ab80d259600cfe/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
jleibs added a commit that referenced this issue Jan 19, 2024
#4867)

### What
This is the basic pattern we expect to see for all of these
view-configuration type tasks:
 - Add a new archetype / component
- Add logic to the TimeSeries space-view-class implementation that both
reads and writes the components directly from the blueprint store.
 
This generally seems to be much easier to think about than dealing with
the EntityProperties struct as this was handled previously as it's
exclusively a contact between the TimeSeries archetype and the
TimeSeriesSpaceView implementation.

Open question:
- Should we suffix all our view archetypes with something like
`TimeSeriesView`?

Needs #4609 to land first since I
developed there before rebasing to main.

Will be nice to come back and clean this up again once
#3384 is done.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4867/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4867/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4867/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4867)
- [Docs
preview](https://rerun.io/preview/d76ed7714f29407bc1d4a62779bfa2894a10fa90/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/d76ed7714f29407bc1d4a62779bfa2894a10fa90/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
@nikolausWest nikolausWest added this to the Triage milestone Jan 30, 2024
@emilk emilk self-assigned this Feb 26, 2024
@emilk emilk mentioned this issue Feb 26, 2024
5 tasks
emilk added a commit that referenced this issue Feb 27, 2024
### What
* Part of #3384

This implements C-style `enum` support in the codegen, with a full Rust
implementation.

There is no example included, because the Python and C++ codegen is
still emitting an error when attempting to use `enum`, but I have tested
the codegen a bit locally, and the generated code at least _compiles_.

Future PRs:
* Python codegen
* C++ codegen
* Tests
* Converting existing types to `enum`

While working on the PR I also cleaned up some other codegen stuff, but
reviewing everything commit-by-commit should make it all obvious.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5291/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5291/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5291/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5291)
- [Docs
preview](https://rerun.io/preview/017812cb07a396308b070e411334edcf2a4cf352/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/017812cb07a396308b070e411334edcf2a4cf352/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
emilk added a commit that referenced this issue Feb 27, 2024
### What
* Part of #3384

### Coming later
* Python
* Tests
* Switching to using `enum` for existing types

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5314/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5314/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5314/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5314)
- [Docs
preview](https://rerun.io/preview/cdd09f81a3a8fa16c2dbbffca7403165bc907739/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/cdd09f81a3a8fa16c2dbbffca7403165bc907739/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Andreas Reich <andreas@rerun.io>
@emilk emilk mentioned this issue Feb 27, 2024
5 tasks
emilk added a commit that referenced this issue Feb 28, 2024
### What
* Part of #3384

Even includes codegen of the arrow serialization, which is a first for
Python.

Best reviewed commit-by-commit

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5319/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5319/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5319/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5319)
- [Docs
preview](https://rerun.io/preview/2f1d162e5f221d9c0a78b1bcff1524fec67e110c/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/2f1d162e5f221d9c0a78b1bcff1524fec67e110c/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
emilk added a commit that referenced this issue Feb 29, 2024
### What
* Closes #3384

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
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5336/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5336/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5336/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5336)
- [Docs
preview](https://rerun.io/preview/5819de7329032052d5291e1696706667009c8fc7/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/5819de7329032052d5291e1696706667009c8fc7/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Clement Rey <cr.rey.clement@gmail.com>
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 a pull request may close this issue.

5 participants