-
Notifications
You must be signed in to change notification settings - Fork 317
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
Query editor for the dataframe view #7071
Conversation
2ae9325
to
17e321c
Compare
The `DataframeQuery` archetype does _not_ include selected components nor point-of-view components for the time being. Lots of TODO/WIP
17e321c
to
c9fd2d3
Compare
impl Default for LatestAtQuery { | ||
fn default() -> Self { | ||
Self { | ||
timeline: Utf8::from("log_time"), | ||
time: TimeInt::MAX, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default is neither useful, nor very robust (hardcoded timeline name), but required for reflection to work.
crates/store/re_types/definitions/rerun/blueprint/components/timeline.fbs
Outdated
Show resolved
Hide resolved
impl Default for TimeRangeQuery { | ||
fn default() -> Self { | ||
Self { | ||
timeline: Utf8::from("log_time"), | ||
start: TimeInt::MIN, | ||
end: TimeInt::MAX, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, unfortunate Default
to be having.
else { | ||
re_log::warn_once!("Could not find timeline {:?}.", timeline_name.as_str()); | ||
//TODO(ab): we should have an error for that | ||
return Ok(()); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SpaceViewSystemExecutionError
return type is a bit rigid for the various error situation one might encounter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add a SpaceViewSystemExecutionError::Custom
maybe?
// The presence (or not) of the timeline component determines if the view should follow the | ||
// time panel timeline/latest at query, or override it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How bad is that going to bite me when doing the blueprint api?
from __future__ import annotations | ||
|
||
from ... import datatypes | ||
|
||
|
||
class LatestAtQueryExt: | ||
"""Extension for [LatestAtQuery][rerun.blueprint.datatypes.LatestAtQuery].""" | ||
|
||
# This override is required because otherwise the codegen uses `TimeInt(x)`, which is not valid with the custom | ||
# `TimeInt.__init__` override. | ||
|
||
@staticmethod | ||
def time__field_converter_override(x: datatypes.TimeIntLike) -> datatypes.TimeInt: | ||
if isinstance(x, datatypes.TimeInt): | ||
return x | ||
else: | ||
return datatypes.TimeInt(seq=x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a ton to mypy
because I wasn't gonna catch this one by myself 😅
…and-ui # Conflicts: # crates/store/re_types/definitions/rerun/blueprint/components/dataframe_view_mode.fbs # rerun_cpp/src/rerun/blueprint/archetypes/dataframe_view_mode.cpp # rerun_cpp/src/rerun/blueprint/archetypes/dataframe_view_mode.hpp
crates/store/re_types/definitions/rerun/blueprint/archetypes/dataframe_query.fbs
Outdated
Show resolved
Hide resolved
crates/store/re_types/definitions/rerun/blueprint/archetypes/dataframe_query.fbs
Outdated
Show resolved
Hide resolved
crates/store/re_types/definitions/rerun/blueprint/archetypes/dataframe_query.fbs
Outdated
Show resolved
Hide resolved
crates/store/re_types/definitions/rerun/blueprint/components/timeline.fbs
Outdated
Show resolved
Hide resolved
crates/store/re_types/definitions/rerun/blueprint/datatypes/time_range_query.fbs
Outdated
Show resolved
Hide resolved
crates/store/re_types/definitions/rerun/blueprint/components/timeline.fbs
Outdated
Show resolved
Hide resolved
else { | ||
re_log::warn_once!("Could not find timeline {:?}.", timeline_name.as_str()); | ||
//TODO(ab): we should have an error for that | ||
return Ok(()); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add a SpaceViewSystemExecutionError::Custom
maybe?
Have a few comments about this, however they may not belong to this particular issue. I leave that up to you!
follow.override.mov
My guess this could be a part of this issue: #6708 |
For (1), I tried and it somehow gets super ugly with the order/sort UI next to it. I will postpone to when we deal with that (we'll have to discuss the exact UI at that point anyways):
I've made some adjustment for (2). |
### What Main broken by #7128, which missed an updated to code recently introduced (#7071) ### 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/7139?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/7139?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/7139) - [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`.
What
This PR introduce a new blueprint archetype and related bespoke UI to configure the query underlying a dataframe view. As a consequence of this new scheme, Visible Time Range is no longer shown in dataframe view selection panel.
Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerCHANGELOG.md
and the migration guideTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.