rerun icon indicating copy to clipboard operation
rerun copied to clipboard

Fix component reflection such as to not force the component to `impl Default`

Open abey79 opened this issue 1 year ago • 4 comments

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 avatar Sep 18 '24 14:09 abey79

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.

kpreid avatar Sep 18 '24 15:09 kpreid

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 avatar Sep 18 '24 15:09 Wumpf

@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 avatar Sep 18 '24 16:09 Wumpf

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 https://github.com/rerun-io/rerun/pull/7440 .. unless someone does a codegen thing where we check whether Default procmacro is used or there's a Default impl in the extension :)

Wumpf avatar Sep 18 '24 19:09 Wumpf