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

Fix component reflection such as to not force the component to impl Default #7439

Closed
abey79 opened this issue Sep 18, 2024 · 4 comments · Fixed by #7447
Closed

Fix component reflection such as to not force the component to impl Default #7439

abey79 opened this issue Sep 18, 2024 · 4 comments · Fixed by #7447
Labels
😤 annoying Something in the UI / SDK is annoying to use codegen/idl 🦀 Rust API Rust logging API

Comments

@abey79
Copy link
Member

abey79 commented Sep 18, 2024

Currently, the codegen'd component reflection always emit a call to C::default(), forcing the component to impl Default. Sometime this really is meaning less (e.g what's a default EntityPath or ComponentName?).

Funnily the ComponentReflection struct actually has an Option. The codegen should therefore emit None if the component doesn't have Default (which the codegen knows from the attr.rust.derive attribute).

Edit: the codegen doesn't always know about Default if it's manually implemented. So that should be controlled with a new fbs attribute attr.rust.no_placeholder

@abey79 abey79 added 🦀 Rust API Rust logging API 😤 annoying Something in the UI / SDK is annoying to use codegen/idl labels Sep 18, 2024
@kpreid
Copy link
Collaborator

kpreid commented Sep 18, 2024

I ran into this myself when adding ShowLabels. ShowLabels is a component whose fallback value is always computed from other data, and does not have a default that is meaningful at all.

@Wumpf
Copy link
Member

Wumpf commented Sep 18, 2024

We discussed this internally quite a bit internally
https://rerunio.slack.com/archives/C041NHU952S/p1726669945999429

The gist of it that we do need guaranteed defaults for anything that's potentially ui editable (for the value to start out with). Overall, the force to default was introduced as a simplification so that anyone querying a fallback can rely on it being there and treat it as an hard error when it isn't.
It is a known issue that Default trait us such is slightly misleading semantically here. When we introduced this we discussed the possibility of a separate trait but discarded this because of the added structural overhead.

@Wumpf
Copy link
Member

Wumpf commented Sep 18, 2024

@kpreid ShowLabels would be a case where a default value is important. The reason is that one can edit it in the visualizer ui and the ui needs a value to start with.

Edit: to clarify, yes in this case there should always be a fallback provider that computes the proper default. But the design choice here was to protect against what happens if there is none. E.g. someone adds a new visualizer that doesn't specify the fallback provider.
The idea of more hardwired fallback providers floated around as well previously, but it's complex because fallback providers always reason about some context they are invoked in.

@Wumpf
Copy link
Member

Wumpf commented Sep 18, 2024

Have some work in progress that allows to generate placeholders fully generically so we need lot less of those pointless looking defaults and can actually make use of the fact there's always a placeholder 🙂
https://github.com/rerun-io/rerun/tree/andreas/automatic-component-placeholders

since we still want to use Default for custom placeholders when available I think we'll have to make it opt-out as proposed in #7440
.. unless someone does a codegen thing where we check whether Default procmacro is used or there's a Default impl in the extension :)

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 🦀 Rust API Rust logging API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants