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.

Sign up for GitHub

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

Jump to bottom

Move DataQueryResults into the ViewerContext #4310

Merged
merged 13 commits into from
Nov 29, 2023
Merged

Move DataQueryResults into the ViewerContext #4310

merged 13 commits into from
Nov 29, 2023

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Nov 22, 2023 •
edited by github-actions bot
Loading

What

Rather than doing queries on-demand, we now do all the queries up-front when creating the ViewerContext.

This keeps us from re-running duplicate queries, and also eliminates the need to maintain a parallel implementation of resolve instead of just looking up the query results.

Probably best reviewed commit-by-commit.

This already hits some snags related to ViewerContext lifecycle that will hopefully be addressed in #1325

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@jleibs jleibs added 🚜 refactor Change the code, not the functionality 🟦 blueprint The data that defines our UI labels Nov 22, 2023
@jleibs jleibs marked this pull request as ready for review November 22, 2023 20:25
@jleibs jleibs added do-not-merge Do not merge this PR exclude from changelog PRs with this won't show up in CHANGELOG.md labels Nov 22, 2023
This was referenced Nov 22, 2023
Merged
Closed
@jleibs jleibs removed the do-not-merge Do not merge this PR label Nov 28, 2023
@jleibs jleibs force-pushed the jleibs/early_queries branch from 5430d1d to 9eb2c1f Compare November 28, 2023 17:59
@jleibs jleibs changed the base branch from blueprint_refactor to main November 28, 2023 17:59
This was referenced Nov 29, 2023
Merged
Merged
@jleibs jleibs force-pushed the jleibs/early_queries branch from d9bf54e to f7ccf51 Compare November 29, 2023 02:29
@teh-cmc teh-cmc self-requested a review November 29, 2023 07:41
teh-cmc
teh-cmc approved these changes Nov 29, 2023
View reviewed changes
crates/re_viewer_context/src/query_context.rs Outdated Show resolved Hide resolved
crates/re_space_view/src/data_query.rs Outdated Show resolved Hide resolved
@jleibs jleibs force-pushed the jleibs/early_queries branch from f7ccf51 to 2239f89 Compare November 29, 2023 14:29
@jleibs jleibs merged commit ab1490b into main Nov 29, 2023
40 checks passed
@jleibs jleibs deleted the jleibs/early_queries branch November 29, 2023 14:52
jleibs added a commit that referenced this pull request Nov 29, 2023
@jleibs
Fully migrate to DataQueryBlueprint (#4311)
086b681 
### What
- Part of the #4308 stack:
  - #4310
  - THIS PR: #4311
  - #4380
  - #4381

This activates the new expression-based DataQueries and removes the
usages of SpaceViewContents.
Also includes a very primitive query-expression editor.

### Known issues
- #4377 captures several issues, specifically:
- Add/Remove Entity logic is broken. (See:
#4381)
  - Heuristics run way too slowly for complex scenes

However, for the sake of reviewer sanity, that work will be broken out
as a follow-on PR. This PR is going to be painful enough as it is.

### 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 [demo.rerun.io](https://demo.rerun.io/pr/4311) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4311)
- [Docs
preview](https://rerun.io/preview/68b22fecfb162e692db791f53e2cdbce1969b53d/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/68b22fecfb162e692db791f53e2cdbce1969b53d/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
jleibs added a commit that referenced this pull request Nov 29, 2023
@jleibs
Add support for exclusion expressions to the DataQueryBlueprint (#4380)
ecb8e87 
### What
- Part of the #4308 stack:
  - #4310
  - #4311
  - THIS PR: #4380
  - #4381
- Resolves: #4158

The query logic is as follows because it was simple and was sufficient
to mostly match our existing add/remove semantics. It basically
prioritizes exclusions over inclusions. Anything recursively excluded
will be pruned, even if explicitly included. We likely want to change
this behavior, but we can do so in a future PR.

Expands the query evaluator to support exclusions, and UI mechanism for
editing.

![image](https://github.com/rerun-io/rerun/assets/3312232/e35fba49-bbce-48da-8e29-03d0dbb6d985)

### 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 [app.rerun.io](https://app.rerun.io/pr/4380) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4380)
- [Docs
preview](https://rerun.io/preview/45acbc744dddc65ea6682d5c58755ed27e96db78/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/45acbc744dddc65ea6682d5c58755ed27e96db78/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
jleibs added a commit that referenced this pull request Nov 29, 2023
@jleibs
Wire the remove-entity buttons to query exclusions (#4381)
0ab1412 
### What
- Part of the #4308 stack:
  - #4310
  - #4311
  - #4380
  - THIS PR: #4381

Now that we have functioning query exclusions, wire them up to the
remove-entity buttons.

### 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 [app.rerun.io](https://app.rerun.io/pr/4381) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4381)
- [Docs
preview](https://rerun.io/preview/2d33bf3b63da8df3c49e1379ca2af17bd22fc75e/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/2d33bf3b63da8df3c49e1379ca2af17bd22fc75e/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
teh-cmc pushed a commit that referenced this pull request Nov 30, 2023
@jleibs @teh-cmc
Move DataQueryResults into the ViewerContext (#4310)
0ffa61c 
### What
- Part of the #4308 stack:
  - THIS PR: #4310
  - #4311
  - #4380
  - #4381

Rather than doing queries on-demand, we now do all the queries up-front
when creating the ViewerContext.

This keeps us from re-running duplicate queries, and also eliminates the
need to maintain a parallel implementation of resolve instead of just
looking up the query results.

Probably best reviewed commit-by-commit.

This already hits some snags related to ViewerContext lifecycle that
will hopefully be addressed in
#1325

### 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 [demo.rerun.io](https://demo.rerun.io/pr/4310) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4310)
- [Docs
preview](https://rerun.io/preview/2239f89de45fe0ac81fc5660ca13f3b95d7a8799/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/2239f89de45fe0ac81fc5660ca13f3b95d7a8799/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
teh-cmc pushed a commit that referenced this pull request Nov 30, 2023
@jleibs @teh-cmc
Fully migrate to DataQueryBlueprint (#4311)
c2217d0 
### What
- Part of the #4308 stack:
  - #4310
  - THIS PR: #4311
  - #4380
  - #4381

This activates the new expression-based DataQueries and removes the
usages of SpaceViewContents.
Also includes a very primitive query-expression editor.

### Known issues
- #4377 captures several issues, specifically:
- Add/Remove Entity logic is broken. (See:
#4381)
  - Heuristics run way too slowly for complex scenes

However, for the sake of reviewer sanity, that work will be broken out
as a follow-on PR. This PR is going to be painful enough as it is.

### 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 [demo.rerun.io](https://demo.rerun.io/pr/4311) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4311)
- [Docs
preview](https://rerun.io/preview/68b22fecfb162e692db791f53e2cdbce1969b53d/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/68b22fecfb162e692db791f53e2cdbce1969b53d/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
teh-cmc pushed a commit that referenced this pull request Nov 30, 2023
@jleibs @teh-cmc
Add support for exclusion expressions to the DataQueryBlueprint (#4380)
f090adc 
### What
- Part of the #4308 stack:
  - #4310
  - #4311
  - THIS PR: #4380
  - #4381
- Resolves: #4158

The query logic is as follows because it was simple and was sufficient
to mostly match our existing add/remove semantics. It basically
prioritizes exclusions over inclusions. Anything recursively excluded
will be pruned, even if explicitly included. We likely want to change
this behavior, but we can do so in a future PR.

Expands the query evaluator to support exclusions, and UI mechanism for
editing.

![image](https://github.com/rerun-io/rerun/assets/3312232/e35fba49-bbce-48da-8e29-03d0dbb6d985)

### 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 [app.rerun.io](https://app.rerun.io/pr/4380) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4380)
- [Docs
preview](https://rerun.io/preview/45acbc744dddc65ea6682d5c58755ed27e96db78/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/45acbc744dddc65ea6682d5c58755ed27e96db78/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
teh-cmc pushed a commit that referenced this pull request Nov 30, 2023
@jleibs @teh-cmc
Wire the remove-entity buttons to query exclusions (#4381)
5d8b399 
### What
- Part of the #4308 stack:
  - #4310
  - #4311
  - #4380
  - THIS PR: #4381

Now that we have functioning query exclusions, wire them up to the
remove-entity buttons.

### 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 [app.rerun.io](https://app.rerun.io/pr/4381) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4381)
- [Docs
preview](https://rerun.io/preview/2d33bf3b63da8df3c49e1379ca2af17bd22fc75e/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/2d33bf3b63da8df3c49e1379ca2af17bd22fc75e/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
Reviewers

@teh-cmc teh-cmc teh-cmc approved these changes

Assignees
No one assigned
Labels
🟦 blueprint The data that defines our UI exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jleibs @teh-cmc