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

Improve the handling of out-of-DataResultTree data result items #5742

Open
abey79 opened this issue Apr 2, 2024 · 1 comment
Open

Improve the handling of out-of-DataResultTree data result items #5742

abey79 opened this issue Apr 2, 2024 · 1 comment
Labels
ui concerns graphical user interface

Comments

@abey79
Copy link
Contributor

abey79 commented Apr 2, 2024

Background

In #5342, a DataResultNodeOrPath structure was introduced in the blueprint tree UI code address the fact that we want the tree to display $origin even if that actual entity path is not part of the data results. That happens for example when the query is set to something like + $origin/sub_entity/**. In that case, for the purpose of tree UI code, $origin would be a "placeholder" node.

Problem

This has unfortunately wider ranging implications. Since the "placeholder" origin is displayed, it can be selected. However, it is considered "invalid" by some other part of the code (namely ViewportBlueprint::is_item_valid()), which in turns preclude said item to be selected. This caused #5683.

This bug was fixed in #5743 with a hack that is probably fragile (aka tightly coupled to what we want to display in the blueprint tree).

Solution

As per @jleibs's comment below:

  • the query result computed in the AppState::show() should always include the entire $origin subtree
  • items that are in the origin subtree but not actual results from the query should exist, have their tree_prefix_only flag set, be displayed in the blueprint tree, but not in the actual space view
  • it should then be possible to get rid of the DataResultsNodeOrPath enum
  • the fix from Fix issue where "placeholder" DataResults couldn't be selected #5743 should be reverted
@abey79 abey79 added the ui concerns graphical user interface label Apr 2, 2024
@abey79 abey79 changed the title Improve the handling of out-of-DataRestultTree space origin Improve the handling of out-of-DataRestultTree data result items Apr 2, 2024
@abey79 abey79 changed the title Improve the handling of out-of-DataRestultTree data result items Improve the handling of out-of DataRestultTree data result items Apr 2, 2024
@abey79 abey79 changed the title Improve the handling of out-of DataRestultTree data result items Improve the handling of out-of-DataResultTree data result items Apr 2, 2024
@jleibs
Copy link
Member

jleibs commented Apr 2, 2024

The place where we invoke is_item_valid is only a couple lines above the place where we generate the query results. I suspect if move this check to after we run the query, we can use actual query results rather than using the filter. This would let us unify the behavior by making the query results themself the arbiter of whether something is valid.

abey79 added a commit that referenced this issue Apr 2, 2024
### What

- Fixes #5683

This PR fixes an issue where "placeholder" `DataResult` (as per
`DataResultNodeOrPath`) could not be selected because they would be
rejected as invalid by `ViewportBlueprint::is_item_valid()`.

This fix is quite hacky and should be improved:
- #5742

### 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:
[rerun.io/viewer](https://rerun.io/viewer/pr/5743)
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/5743?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/5743?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/5743)
- [Docs
preview](https://rerun.io/preview/e1c43deb543078c413ec716c540255852b6dc429/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/e1c43deb543078c413ec716c540255852b6dc429/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui concerns graphical user interface
Projects
None yet
Development

No branches or pull requests

2 participants