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 VisualizerOverrides serializer and improved error handling #7288

Merged
merged 5 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion crates/build/re_types_builder/src/codegen/python/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,32 @@ impl ExtensionClass {
let has_array = methods.contains(&ARRAY_METHOD);
let has_native_to_pa_array = methods.contains(&NATIVE_TO_PA_ARRAY_METHOD);
let has_deferred_patch_class = methods.contains(&DEFERRED_PATCH_CLASS_METHOD);
let field_converter_overrides = methods
let field_converter_overrides: Vec<String> = methods
.into_iter()
.filter(|l| l.ends_with(FIELD_CONVERTER_SUFFIX))
.map(|l| l.to_owned())
.collect();

let valid_converter_overrides = if obj.is_union() {
itertools::Either::Left(std::iter::once("inner"))
} else {
itertools::Either::Right(obj.fields.iter().map(|field| field.name.as_str()))
}
.map(|field| format!("{field}{FIELD_CONVERTER_SUFFIX}"))
.collect::<HashSet<_>>();

for converter in &field_converter_overrides {
if !valid_converter_overrides.contains(converter) {
reporter.error(
ext_filepath.as_str(),
&obj.fqname,
format!(
"The field converter override `{converter}` is not a valid field name.",
),
);
}
}

Self {
found: true,
file_name,
Expand Down
39 changes: 22 additions & 17 deletions crates/viewer/re_selection_panel/src/visualizer_ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use re_types_blueprint::blueprint::components::VisualizerOverrides;
use re_ui::{list_item, UiExt as _};
use re_viewer_context::{
DataResult, QueryContext, SpaceViewClassExt as _, UiLayout, ViewContext, ViewSystemIdentifier,
VisualizerSystem,
};
use re_viewport_blueprint::SpaceViewBlueprint;

Expand Down Expand Up @@ -111,18 +112,30 @@ pub fn visualizer_ui_impl(

for &visualizer_id in active_visualizers {
let default_open = true;
ui.list_item()
.interactive(false)
.show_hierarchical_with_children(
ui,
ui.make_persistent_id(visualizer_id),
default_open,
list_item::LabelContent::new(visualizer_id.as_str())

// List all components that the visualizer may consume.
if let Ok(visualizer) = ctx.visualizer_collection.get_by_identifier(visualizer_id) {
ui.list_item()
.interactive(false)
.show_hierarchical_with_children(
ui,
ui.make_persistent_id(visualizer_id),
default_open,
list_item::LabelContent::new(visualizer_id.as_str())
.min_desired_width(150.0)
.with_buttons(|ui| remove_visualizer_button(ui, visualizer_id))
.always_show_buttons(true),
|ui| visualizer_components(ctx, ui, data_result, visualizer),
);
} else {
ui.list_item_flat_noninteractive(
list_item::LabelContent::new(format!("{visualizer_id} (unknown visualizer)"))
.weak(true)
.min_desired_width(150.0)
.with_buttons(|ui| remove_visualizer_button(ui, visualizer_id))
.always_show_buttons(true),
|ui| visualizer_components(ctx, ui, data_result, visualizer_id),
);
}
}
});
}
Expand All @@ -141,7 +154,7 @@ fn visualizer_components(
ctx: &ViewContext<'_>,
ui: &mut egui::Ui,
data_result: &DataResult,
visualizer_id: ViewSystemIdentifier,
visualizer: &dyn VisualizerSystem,
) {
// Helper for code below
fn non_empty_component_batch_raw(
Expand All @@ -157,14 +170,6 @@ fn visualizer_components(
}
}

// List all components that the visualizer may consume.
let Ok(visualizer) = ctx.visualizer_collection.get_by_identifier(visualizer_id) else {
re_log::warn!(
"Failed to resolve visualizer identifier {visualizer_id}, to a visualizer implementation"
);
return;
};

let query_info = visualizer.visualizer_query_info();

let store_query = ctx.current_query();
Expand Down
4 changes: 3 additions & 1 deletion rerun_py/rerun_sdk/rerun/blueprint/datatypes/utf8list.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class Utf8ListExt:
"""Extension for [Utf8List][rerun.blueprint.datatypes.Utf8List]."""

@staticmethod
def visualizers__field_converter_override(value: str | list[str]) -> list[str]:
def value__field_converter_override(value: str | list[str]) -> list[str]:
if isinstance(value, str):
return [value]
return value
Expand Down
13 changes: 13 additions & 0 deletions rerun_py/tests/unit/test_utf8list.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import rerun.blueprint.components as components
import rerun.blueprint.datatypes as datatypes


Expand Down Expand Up @@ -37,6 +38,7 @@ def test_utf8list() -> None:
single_string = "Hello"
array_of_single_string = [single_string]
array_of_array_of_single_string = [array_of_single_string]

assert (
datatypes.Utf8ListBatch(single_string).as_arrow_array()
== datatypes.Utf8ListBatch(array_of_single_string).as_arrow_array()
Expand All @@ -46,3 +48,14 @@ def test_utf8list() -> None:
datatypes.Utf8ListBatch(array_of_single_string).as_arrow_array()
== datatypes.Utf8ListBatch(array_of_array_of_single_string).as_arrow_array()
)

# A component delegating through to the underlying datatype should behave the same
assert (
components.VisualizerOverrides(single_string).as_arrow_array().storage
== datatypes.Utf8ListBatch(array_of_array_of_single_string).as_arrow_array().storage
)

assert (
components.VisualizerOverrides(list_with_two_strings).as_arrow_array().storage
== datatypes.Utf8ListBatch(list_of_list_with_two_strings).as_arrow_array().storage
)
71 changes: 71 additions & 0 deletions tests/python/release_checklist/check_blueprint_overrides.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
from __future__ import annotations

import os
from argparse import Namespace
from uuid import uuid4

import rerun as rr
import rerun.blueprint as rrb

README = """\
# Blueprint overrides
This checks that overrides work as expected when sent via blueprint APIs.
Expected behavior:
* The `sin` plot should be a blue line (set via defaults)
* The `cos` plot should be a green points with cross markers (set via overrides)
"""


def log_readme() -> None:
rr.log("readme", rr.TextDocument(README, media_type=rr.MediaType.MARKDOWN), static=True)


def log_plots() -> None:
from math import cos, sin, tau

for t in range(0, int(tau * 2 * 10.0)):
rr.set_time_sequence("frame_nr", t)

sin_of_t = sin(float(t) / 10.0)
rr.log("plots/sin", rr.Scalar(sin_of_t))

cos_of_t = cos(float(t) / 10.0)
rr.log("plots/cos", rr.Scalar(cos_of_t))

rr.send_blueprint(
rrb.Blueprint(
rrb.Grid(
rrb.TextDocumentView(origin="readme", name="Instructions"),
rrb.TimeSeriesView(
name="Plots",
defaults=[rr.components.Color([0, 0, 255])],
overrides={
"plots/cos": [
rrb.VisualizerOverrides("SeriesPoint"),
rr.components.Color([0, 255, 0]),
# TODDO(#6670): This should just be `rr.components.MarkerShape.Cross`
rr.components.MarkerShapeBatch("cross"),
],
},
),
)
)
)


def run(args: Namespace) -> None:
rr.script_setup(args, f"{os.path.basename(__file__)}", recording_id=uuid4())

log_readme()
log_plots()


if __name__ == "__main__":
import argparse

parser = argparse.ArgumentParser(description="Interactive release checklist")
rr.script_add_args(parser)
args = parser.parse_args()
run(args)
Loading