Skip to content

Commit 2c4000c

Browse files
authored
Fix VisualizerOverrides serializer and improved error handling (#7288)
### What - Resolves: #7287 The field-converter ended up misnamed likely during a component type refactor. - Add new unit-test for the specific case - Add new check for valid field converts. - Fix the field converter itself - Add a release checklist that uses the override functionality. Also if VisualizerOverrides is bad, we now handle it more gracefully instead of spamming warnings. ![image](https://github.com/user-attachments/assets/5d557e25-8e09-40a4-9afe-251b95e9d4b4) ### 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 examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7288?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7288?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)! * [x] If have noted any breaking changes to the log API in `CHANGELOG.md` and the migration guide - [PR Build Summary](https://build.rerun.io/pr/7288) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`.
1 parent 88b19f9 commit 2c4000c

File tree

6 files changed

+131
-20
lines changed

6 files changed

+131
-20
lines changed

crates/build/re_types_builder/src/codegen/python/mod.rs

+21-1
Original file line numberDiff line numberDiff line change
@@ -240,12 +240,32 @@ impl ExtensionClass {
240240
let has_array = methods.contains(&ARRAY_METHOD);
241241
let has_native_to_pa_array = methods.contains(&NATIVE_TO_PA_ARRAY_METHOD);
242242
let has_deferred_patch_class = methods.contains(&DEFERRED_PATCH_CLASS_METHOD);
243-
let field_converter_overrides = methods
243+
let field_converter_overrides: Vec<String> = methods
244244
.into_iter()
245245
.filter(|l| l.ends_with(FIELD_CONVERTER_SUFFIX))
246246
.map(|l| l.to_owned())
247247
.collect();
248248

249+
let valid_converter_overrides = if obj.is_union() {
250+
itertools::Either::Left(std::iter::once("inner"))
251+
} else {
252+
itertools::Either::Right(obj.fields.iter().map(|field| field.name.as_str()))
253+
}
254+
.map(|field| format!("{field}{FIELD_CONVERTER_SUFFIX}"))
255+
.collect::<HashSet<_>>();
256+
257+
for converter in &field_converter_overrides {
258+
if !valid_converter_overrides.contains(converter) {
259+
reporter.error(
260+
ext_filepath.as_str(),
261+
&obj.fqname,
262+
format!(
263+
"The field converter override `{converter}` is not a valid field name.",
264+
),
265+
);
266+
}
267+
}
268+
249269
Self {
250270
found: true,
251271
file_name,

crates/viewer/re_selection_panel/src/visualizer_ui.rs

+22-17
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use re_types_blueprint::blueprint::components::VisualizerOverrides;
1010
use re_ui::{list_item, UiExt as _};
1111
use re_viewer_context::{
1212
DataResult, QueryContext, SpaceViewClassExt as _, UiLayout, ViewContext, ViewSystemIdentifier,
13+
VisualizerSystem,
1314
};
1415
use re_viewport_blueprint::SpaceViewBlueprint;
1516

@@ -111,18 +112,30 @@ pub fn visualizer_ui_impl(
111112

112113
for &visualizer_id in active_visualizers {
113114
let default_open = true;
114-
ui.list_item()
115-
.interactive(false)
116-
.show_hierarchical_with_children(
117-
ui,
118-
ui.make_persistent_id(visualizer_id),
119-
default_open,
120-
list_item::LabelContent::new(visualizer_id.as_str())
115+
116+
// List all components that the visualizer may consume.
117+
if let Ok(visualizer) = ctx.visualizer_collection.get_by_identifier(visualizer_id) {
118+
ui.list_item()
119+
.interactive(false)
120+
.show_hierarchical_with_children(
121+
ui,
122+
ui.make_persistent_id(visualizer_id),
123+
default_open,
124+
list_item::LabelContent::new(visualizer_id.as_str())
125+
.min_desired_width(150.0)
126+
.with_buttons(|ui| remove_visualizer_button(ui, visualizer_id))
127+
.always_show_buttons(true),
128+
|ui| visualizer_components(ctx, ui, data_result, visualizer),
129+
);
130+
} else {
131+
ui.list_item_flat_noninteractive(
132+
list_item::LabelContent::new(format!("{visualizer_id} (unknown visualizer)"))
133+
.weak(true)
121134
.min_desired_width(150.0)
122135
.with_buttons(|ui| remove_visualizer_button(ui, visualizer_id))
123136
.always_show_buttons(true),
124-
|ui| visualizer_components(ctx, ui, data_result, visualizer_id),
125137
);
138+
}
126139
}
127140
});
128141
}
@@ -141,7 +154,7 @@ fn visualizer_components(
141154
ctx: &ViewContext<'_>,
142155
ui: &mut egui::Ui,
143156
data_result: &DataResult,
144-
visualizer_id: ViewSystemIdentifier,
157+
visualizer: &dyn VisualizerSystem,
145158
) {
146159
// Helper for code below
147160
fn non_empty_component_batch_raw(
@@ -157,14 +170,6 @@ fn visualizer_components(
157170
}
158171
}
159172

160-
// List all components that the visualizer may consume.
161-
let Ok(visualizer) = ctx.visualizer_collection.get_by_identifier(visualizer_id) else {
162-
re_log::warn!(
163-
"Failed to resolve visualizer identifier {visualizer_id}, to a visualizer implementation"
164-
);
165-
return;
166-
};
167-
168173
let query_info = visualizer.visualizer_query_info();
169174

170175
let store_query = ctx.current_query();

rerun_py/rerun_sdk/rerun/blueprint/datatypes/utf8list.py

+3-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

rerun_py/rerun_sdk/rerun/blueprint/datatypes/utf8list_ext.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ class Utf8ListExt:
1212
"""Extension for [Utf8List][rerun.blueprint.datatypes.Utf8List]."""
1313

1414
@staticmethod
15-
def visualizers__field_converter_override(value: str | list[str]) -> list[str]:
15+
def value__field_converter_override(value: str | list[str]) -> list[str]:
1616
if isinstance(value, str):
1717
return [value]
1818
return value

rerun_py/tests/unit/test_utf8list.py

+13
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from __future__ import annotations
22

3+
import rerun.blueprint.components as components
34
import rerun.blueprint.datatypes as datatypes
45

56

@@ -37,6 +38,7 @@ def test_utf8list() -> None:
3738
single_string = "Hello"
3839
array_of_single_string = [single_string]
3940
array_of_array_of_single_string = [array_of_single_string]
41+
4042
assert (
4143
datatypes.Utf8ListBatch(single_string).as_arrow_array()
4244
== datatypes.Utf8ListBatch(array_of_single_string).as_arrow_array()
@@ -46,3 +48,14 @@ def test_utf8list() -> None:
4648
datatypes.Utf8ListBatch(array_of_single_string).as_arrow_array()
4749
== datatypes.Utf8ListBatch(array_of_array_of_single_string).as_arrow_array()
4850
)
51+
52+
# A component delegating through to the underlying datatype should behave the same
53+
assert (
54+
components.VisualizerOverrides(single_string).as_arrow_array().storage
55+
== datatypes.Utf8ListBatch(array_of_array_of_single_string).as_arrow_array().storage
56+
)
57+
58+
assert (
59+
components.VisualizerOverrides(list_with_two_strings).as_arrow_array().storage
60+
== datatypes.Utf8ListBatch(list_of_list_with_two_strings).as_arrow_array().storage
61+
)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
from __future__ import annotations
2+
3+
import os
4+
from argparse import Namespace
5+
from uuid import uuid4
6+
7+
import rerun as rr
8+
import rerun.blueprint as rrb
9+
10+
README = """\
11+
# Blueprint overrides
12+
13+
This checks that overrides work as expected when sent via blueprint APIs.
14+
15+
Expected behavior:
16+
* The `sin` plot should be a blue line (set via defaults)
17+
* The `cos` plot should be a green points with cross markers (set via overrides)
18+
"""
19+
20+
21+
def log_readme() -> None:
22+
rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), static=True)
23+
24+
25+
def log_plots() -> None:
26+
from math import cos, sin, tau
27+
28+
for t in range(0, int(tau * 2 * 10.0)):
29+
rr.set_time_sequence("frame_nr", t)
30+
31+
sin_of_t = sin(float(t) / 10.0)
32+
rr.log("plots/sin", rr.Scalar(sin_of_t))
33+
34+
cos_of_t = cos(float(t) / 10.0)
35+
rr.log("plots/cos", rr.Scalar(cos_of_t))
36+
37+
rr.send_blueprint(
38+
rrb.Blueprint(
39+
rrb.Grid(
40+
rrb.TextDocumentView(origin="readme", name="Instructions"),
41+
rrb.TimeSeriesView(
42+
name="Plots",
43+
defaults=[rr.components.Color([0, 0, 255])],
44+
overrides={
45+
"plots/cos": [
46+
rrb.VisualizerOverrides("SeriesPoint"),
47+
rr.components.Color([0, 255, 0]),
48+
# TODDO(#6670): This should just be `rr.components.MarkerShape.Cross`
49+
rr.components.MarkerShapeBatch("cross"),
50+
],
51+
},
52+
),
53+
)
54+
)
55+
)
56+
57+
58+
def run(args: Namespace) -> None:
59+
rr.script_setup(args, f"{os.path.basename(__file__)}", recording_id=uuid4())
60+
61+
log_readme()
62+
log_plots()
63+
64+
65+
if __name__ == "__main__":
66+
import argparse
67+
68+
parser = argparse.ArgumentParser(description="Interactive release checklist")
69+
rr.script_add_args(parser)
70+
args = parser.parse_args()
71+
run(args)

0 commit comments

Comments
 (0)