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

components::ColorRGBA should use an array #2782

Open
kpreid opened this issue Jul 23, 2023 · 4 comments
Open

components::ColorRGBA should use an array #2782

kpreid opened this issue Jul 23, 2023 · 4 comments
Labels
😤 annoying Something in the UI / SDK is annoying to use codegen/idl 🍏 primitives Relating to Rerun primitives 🦀 Rust API Rust logging API

Comments

@kpreid
Copy link
Collaborator

kpreid commented Jul 23, 2023

This is a Rust API design nitpick.

As of rerun 0.7.0, rerun::components::ColorRGBA publicly uses a u32 to store the color. This is an unnecessary complication, because u32s have endianness which confuses the question of which color component ordering in memory is actually meant. (For example, on little-endian architectures — most computers today — the components are actually stored as what someone discussing image formats would call ABGR, not RGBA.) In my opinion, the representation should actually be either [u32; 4] (with an explicitly documented meaning of each array index) or a struct

#[repr(C)]
struct ColorRGBA {
    r: u8,
    g: u8,
    b: u8,
    a: u8,
}

Either one of these nails down the memory representation unambiguously, and will be representation-compatible (i.e. transmutable) with someone else's RGBA color type when it's actually RGBA and not ABGR.

Another option is to make the existing u32 field private, so that the interpretation of it is entirely an implementation detail of rerun.

@kpreid kpreid added other Generated by the "Other" issue template 👀 needs triage This issue needs to be triaged by the Rerun team labels Jul 23, 2023
@Wumpf Wumpf added 🦀 Rust API Rust logging API 😤 annoying Something in the UI / SDK is annoying to use 🍏 primitives Relating to Rerun primitives and removed other Generated by the "Other" issue template labels Jul 26, 2023
@Wumpf
Copy link
Member

Wumpf commented Jul 26, 2023

agreed! Either the struct as suggested or rgba: [u8; 4] would be an improvement

@emilk emilk added codegen/idl and removed 👀 needs triage This issue needs to be triaged by the Rerun team labels Aug 8, 2023
@emilk
Copy link
Member

emilk commented Aug 8, 2023

I think [u8; 4] is the best choice here (especially since Arrow will split a struct into separate columns), with nice helper constructors.

@emilk emilk added the 🏎️ Quick Issue Can be fixed in a few hours or less label Sep 11, 2023
@emilk
Copy link
Member

emilk commented Sep 11, 2023

Let's improve the docs for 0.9, but not change the type until 0.10

@emilk emilk self-assigned this Sep 12, 2023
emilk added a commit that referenced this issue Sep 12, 2023
### What
* Related to #2782

### 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 [demo.rerun.io](https://demo.rerun.io/pr/3289) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/3289)
- [Docs
preview](https://rerun.io/preview/cbd599a8e94ff31bff87f7ab42d5a6ac441b3d0d/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/cbd599a8e94ff31bff87f7ab42d5a6ac441b3d0d/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
@emilk emilk removed this from the 0.9 - Codegen, Type Oriented Api milestone Sep 12, 2023
@emilk
Copy link
Member

emilk commented Sep 12, 2023

I've improved the docs now, but will leave this issue open as a reminder to change the implementation to the more natural [u8; 4]

@emilk emilk removed their assignment Sep 19, 2023
@Wumpf Wumpf changed the title components::ColorRGBA should either document how the public u32 works, or use an array components::ColorRGBA should use an array Dec 19, 2023
@Wumpf Wumpf removed the 🏎️ Quick Issue Can be fixed in a few hours or less label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😤 annoying Something in the UI / SDK is annoying to use codegen/idl 🍏 primitives Relating to Rerun primitives 🦀 Rust API Rust logging API
Projects
None yet
Development

No branches or pull requests

4 participants