Skip to content

Commit

Permalink
Remove unnecessary lifetime from ViewProperty (#7514)
Browse files Browse the repository at this point in the history
### What

Makes `ViewProperty` more ergonomic to use at the cost of an additional
clone of `LatestAtQuery` (which is very cheap).

### 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/7514?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/7514?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/7514)
- [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`.
  • Loading branch information
abey79 authored Sep 25, 2024
1 parent 186dc50 commit 14ff694
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 31 deletions.
2 changes: 1 addition & 1 deletion crates/viewer/re_space_view_dataframe/src/view_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ fn override_ui(
ui: &mut egui::Ui,
state: &dyn SpaceViewState,
space_view_id: SpaceViewId,
property: &ViewProperty<'_>,
property: &ViewProperty,
) -> Result<(), SpaceViewSystemExecutionError> {
ui.add_space(4.0);
egui::Grid::new("dataframe_view_query_ui")
Expand Down
2 changes: 1 addition & 1 deletion crates/viewer/re_space_view_spatial/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ fn query_pinhole_legacy(

pub(crate) fn configure_background(
ctx: &ViewerContext<'_>,
background: &ViewProperty<'_>,
background: &ViewProperty,
render_ctx: &RenderContext,
view_system: &dyn re_viewer_context::ComponentFallbackProvider,
state: &dyn re_viewer_context::SpaceViewState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use re_viewport_blueprint::ViewProperty;
/// * out of bounds dimensions and indices are clamped to valid
/// * missing width/height is filled in if there's at least 2 dimensions.
pub fn load_tensor_slice_selection_and_make_valid(
slice_selection: &ViewProperty<'_>,
slice_selection: &ViewProperty,
shape: &[TensorDimension],
) -> Result<TensorSliceSelection, re_types::DeserializationError> {
re_tracing::profile_function!();
Expand Down
2 changes: 1 addition & 1 deletion crates/viewer/re_space_view_tensor/src/space_view_class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ fn selectors_ui(
ui: &mut egui::Ui,
shape: &[TensorDimension],
slice_selection: &TensorSliceSelection,
slice_property: &ViewProperty<'_>,
slice_property: &ViewProperty,
) {
let Some(slider) = &slice_selection.slider else {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl DragDropAddress {
&self,
ctx: &ViewerContext<'_>,
slice_selection: &TensorSliceSelection,
slice_property: &ViewProperty<'_>,
slice_property: &ViewProperty,
new_selection: Option<TensorDimensionIndexSelection>,
) {
match self {
Expand Down Expand Up @@ -148,7 +148,7 @@ pub fn dimension_mapping_ui(
ui: &mut egui::Ui,
shape: &[TensorDimension],
slice_selection: &TensorSliceSelection,
slice_property: &ViewProperty<'_>,
slice_property: &ViewProperty,
) {
let mut drag_source = DragDropAddress::None; // Drag this…
let mut drop_target = DragDropAddress::None; // …onto this.
Expand Down
50 changes: 25 additions & 25 deletions crates/viewer/re_viewport_blueprint/src/view_properties.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,36 +28,36 @@ impl From<ViewPropertyQueryError> for SpaceViewSystemExecutionError {
}

/// Utility for querying view properties.
pub struct ViewProperty<'a> {
pub struct ViewProperty {
/// Entity path in the blueprint store where all components of this view property archetype are
/// stored.
pub blueprint_store_path: EntityPath,

archetype_name: ArchetypeName,
component_names: Vec<ComponentName>,
query_results: LatestAtResults,
blueprint_query: &'a LatestAtQuery,
blueprint_query: LatestAtQuery,
}

impl<'a> ViewProperty<'a> {
impl ViewProperty {
/// Query a specific view property for a given view.
pub fn from_archetype<A: Archetype>(
blueprint_db: &'a EntityDb,
blueprint_query: &'a LatestAtQuery,
blueprint_db: &EntityDb,
blueprint_query: &LatestAtQuery,
view_id: SpaceViewId,
) -> Self {
Self::from_archetype_impl(
blueprint_db,
blueprint_query,
blueprint_query.clone(),
view_id,
A::name(),
A::all_components().as_ref(),
)
}

fn from_archetype_impl(
blueprint_db: &'a EntityDb,
blueprint_query: &'a LatestAtQuery,
blueprint_db: &EntityDb,
blueprint_query: LatestAtQuery,
space_view_id: SpaceViewId,
archetype_name: ArchetypeName,
component_names: &[ComponentName],
Expand All @@ -66,12 +66,12 @@ impl<'a> ViewProperty<'a> {
entity_path_for_view_property(space_view_id, blueprint_db.tree(), archetype_name);

let query_results = blueprint_db.latest_at(
blueprint_query,
&blueprint_query,
&blueprint_store_path,
component_names.iter().copied(),
);

ViewProperty {
Self {
blueprint_store_path,
archetype_name,
query_results,
Expand All @@ -85,9 +85,9 @@ impl<'a> ViewProperty<'a> {
// This sadly means that there's a bit of unnecessary back and forth between arrow array and untyped that could be avoided otherwise.
pub fn component_or_fallback<C: re_types::Component + Default>(
&self,
ctx: &'a ViewerContext<'a>,
ctx: &ViewerContext<'_>,
fallback_provider: &dyn ComponentFallbackProvider,
view_state: &'a dyn re_viewer_context::SpaceViewState,
view_state: &dyn re_viewer_context::SpaceViewState,
) -> Result<C, ViewPropertyQueryError> {
self.component_array_or_fallback::<C>(ctx, fallback_provider, view_state)?
.into_iter()
Expand All @@ -98,9 +98,9 @@ impl<'a> ViewProperty<'a> {
/// Get the component array for a given type or its fallback if the component is not present or empty.
pub fn component_array_or_fallback<C: re_types::Component + Default>(
&self,
ctx: &'a ViewerContext<'a>,
ctx: &ViewerContext<'_>,
fallback_provider: &dyn ComponentFallbackProvider,
view_state: &'a dyn re_viewer_context::SpaceViewState,
view_state: &dyn re_viewer_context::SpaceViewState,
) -> Result<Vec<C>, ViewPropertyQueryError> {
let component_name = C::name();
Ok(C::from_arrow(
Expand Down Expand Up @@ -153,10 +153,10 @@ impl<'a> ViewProperty<'a> {

fn component_or_fallback_raw(
&self,
ctx: &'a ViewerContext<'a>,
ctx: &ViewerContext<'_>,
component_name: ComponentName,
fallback_provider: &dyn ComponentFallbackProvider,
view_state: &'a dyn re_viewer_context::SpaceViewState,
view_state: &dyn re_viewer_context::SpaceViewState,
) -> Result<Box<dyn arrow2::array::Array>, ComponentFallbackError> {
if let Some(value) = self.component_raw(component_name) {
if value.len() > 0 {
Expand All @@ -169,27 +169,27 @@ impl<'a> ViewProperty<'a> {
/// Save change to a blueprint component.
pub fn save_blueprint_component(
&self,
ctx: &'a ViewerContext<'a>,
ctx: &ViewerContext<'_>,
components: &dyn ComponentBatch,
) {
ctx.save_blueprint_component(&self.blueprint_store_path, components);
}

/// Resets a blueprint component to the value it had in the default blueprint.
pub fn reset_blueprint_component<C: re_types::Component>(&self, ctx: &'a ViewerContext<'a>) {
pub fn reset_blueprint_component<C: re_types::Component>(&self, ctx: &ViewerContext<'_>) {
ctx.reset_blueprint_component_by_name(&self.blueprint_store_path, C::name());
}

/// Resets all components to the values they had in the default blueprint.
pub fn reset_all_components(&self, ctx: &'a ViewerContext<'a>) {
pub fn reset_all_components(&self, ctx: &ViewerContext<'_>) {
// Don't use `self.query_results.components.keys()` since it may already have some components missing since they didn't show up in the query.
for &component_name in &self.component_names {
ctx.reset_blueprint_component_by_name(&self.blueprint_store_path, component_name);
}
}

/// Resets all components to empty values, i.e. the fallback.
pub fn reset_all_components_to_empty(&self, ctx: &'a ViewerContext<'a>) {
pub fn reset_all_components_to_empty(&self, ctx: &ViewerContext<'_>) {
for &component_name in self.query_results.components.keys() {
ctx.clear_blueprint_component_by_name(&self.blueprint_store_path, component_name);
}
Expand All @@ -204,16 +204,16 @@ impl<'a> ViewProperty<'a> {
}

/// Create a query context for this view property.
pub fn query_context(
&self,
viewer_ctx: &'a ViewerContext<'a>,
pub fn query_context<'a>(
&'a self,
viewer_ctx: &'a ViewerContext<'_>,
view_state: &'a dyn re_viewer_context::SpaceViewState,
) -> QueryContext<'_> {
) -> QueryContext<'a> {
QueryContext {
viewer_ctx,
target_entity_path: &self.blueprint_store_path,
archetype_name: Some(self.archetype_name),
query: self.blueprint_query,
query: &self.blueprint_query,
view_state,
view_ctx: None,
}
Expand Down

0 comments on commit 14ff694

Please sign in to comment.