Skip to content

Commit

Permalink
Fix a bug where an incorrect 3D space view origin could be suggested (#…
Browse files Browse the repository at this point in the history
…5565)

### What

Fix a bug introduced in #5411 and #5423 where the wrong origin would be
suggested when creating a 3D space view from an entity part of a 2D
space view. Quoting the original PR:

> For 3D space views: the origin of the subspace that contains the
selected entities common ancestor, _or_, if that one is rooted on a
pinhole, the origin of the parent sub-space. Further, if a
`ViewCoordinate` is logged in between the subspace origin and the common
ancestor, then it's used as suggested origin. In most case, that means
`/`, unless a `DisconnectedSpace` is encountered.

The part of that logic where we check for a `ViewCoordinate` in the
parent subspace was implemented incorrectly, which resulted in
`check_context_menu_suggested_origin` to fail. With this PR, this test
now passes.

### 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 newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5565/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5565/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5565/index.html?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)!

- [PR Build Summary](https://build.rerun.io/pr/5565)
- [Docs
preview](https://rerun.io/preview/268125aaddc63e0524743528cf3b34c9ea0640e3/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/268125aaddc63e0524743528cf3b34c9ea0640e3/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
  • Loading branch information
abey79 authored Mar 18, 2024
1 parent 1385bcd commit cb19989
Showing 1 changed file with 14 additions and 10 deletions.
24 changes: 14 additions & 10 deletions crates/re_space_view_spatial/src/space_view_3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,16 @@ impl SpaceViewClass for SpatialSpaceView3D {
// Also, if a ViewCoordinate3D is logged somewhere between the common ancestor and the
// subspace origin, we use it as origin.
SpatialTopology::access(entity_db.store_id(), |topo| {
let subspace = topo.subspace_for_entity(&common_ancestor);
let common_ancestor_subspace = topo.subspace_for_entity(&common_ancestor);

let subspace_origin = if subspace.supports_3d_content() {
Some(subspace)
// Consider the case where the common ancestor might be in a 2D space that is connected
// to a parent space. In this case, the parent space is the correct space.
let subspace = if common_ancestor_subspace.supports_3d_content() {
Some(common_ancestor_subspace)
} else {
topo.subspace_for_subspace_origin(subspace.parent_space)
}
.map(|subspace| subspace.origin.clone());
topo.subspace_for_subspace_origin(common_ancestor_subspace.parent_space)
};
let subspace_origin = subspace.map(|subspace| subspace.origin.clone());

// Find the first ViewCoordinates3d logged, walking up from the common ancestor to the
// subspace origin.
Expand All @@ -114,10 +116,12 @@ impl SpaceViewClass for SpatialSpaceView3D {
.into_iter()
.rev()
.find(|path| {
subspace
.heuristic_hints
.get(path)
.is_some_and(|hint| hint.contains(HeuristicHints::ViewCoordinates3d))
subspace.is_some_and(|subspace| {
subspace
.heuristic_hints
.get(path)
.is_some_and(|hint| hint.contains(HeuristicHints::ViewCoordinates3d))
})
})
.or(subspace_origin)
})
Expand Down

0 comments on commit cb19989

Please sign in to comment.