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

Implement incremental graph layouts #8308

Merged
merged 9 commits into from
Dec 4, 2024
Merged

Implement incremental graph layouts #8308

merged 9 commits into from
Dec 4, 2024

Conversation

grtlr
Copy link
Contributor

@grtlr grtlr commented Dec 4, 2024

Related

What

We made the decision to carry over layout information between timestamps as it leads to a much nicer user experience. This PR implements that feature.

@abey79 Some of the logic in provider.rs has to change again for blueprint support. I plan to do a cleanup pass in #8299.

Screen.Recording.2024-12-04.at.09.44.00.mov

@grtlr grtlr added ui concerns graphical user interface include in changelog feat-graph-view Everything related to the graph view labels Dec 4, 2024
Copy link

github-actions bot commented Dec 4, 2024

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link
d4ba371 https://rerun.io/viewer/pr/8308

Note: This comment is updated whenever you push a commit.

@grtlr grtlr requested a review from abey79 December 4, 2024 13:17
Copy link
Member

@abey79 abey79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Comment on lines +113 to +125
Self::Finished { layout, .. } => {
let mut provider = ForceLayoutProvider::new_with_previous(new_request, &layout);
let mut layout = provider.init();
provider.tick(&mut layout);

Self::InProgress { layout, provider }
}
Self::InProgress {
layout, provider, ..
} if provider.request != new_request => {
let mut provider = ForceLayoutProvider::new_with_previous(new_request, &layout);
let mut layout = provider.init();
provider.tick(&mut layout);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two match arms look like they can be lumped together:

Suggested change
Self::Finished { layout, .. } => {
let mut provider = ForceLayoutProvider::new_with_previous(new_request, &layout);
let mut layout = provider.init();
provider.tick(&mut layout);
Self::InProgress { layout, provider }
}
Self::InProgress {
layout, provider, ..
} if provider.request != new_request => {
let mut provider = ForceLayoutProvider::new_with_previous(new_request, &layout);
let mut layout = provider.init();
provider.tick(&mut layout);
Self::Finished { layout, .. } | Self::InProgress {
layout, provider, ..
} if provider.request != new_request => {
let mut provider = ForceLayoutProvider::new_with_previous(new_request, &layout);
let mut layout = provider.init();
provider.tick(&mut layout);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's possible due to the if-guard on the Self::InProgress arm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok. I didn't know there was such a restriction.

tests/python/release_checklist/check_graph_time_layout.py Outdated Show resolved Hide resolved
tests/python/release_checklist/check_graph_time_layout.py Outdated Show resolved Hide resolved
grtlr and others added 6 commits December 4, 2024 17:31
Co-authored-by: Antoine Beyeler <49431240+abey79@users.noreply.github.com>
### Related

* split out of #8234 

### What

* use `ViewProperty` util construct some more
* route individual property ui via `view_property_component_ui_custom`
making it easier to draw custom ui for specific view properties while
still having all tooltip & extra menus be the same
### What

Fixes CI problems
(https://github.com/rerun-io/rerun/actions/runs/12160549310/job/33913118372?pr=8313):

```
error[vulnerability]: Borsh serialization of HashMap is non-canonical
    ┌─ /home/runner/work/rerun/rerun/Cargo.lock:225:1
    │
225 │ hashbrown 0.15.0 registry+https://github.com/rust-lang/crates.io-index
    │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ security vulnerability detected
    │
    ├ ID: RUSTSEC-2024-0402
    ├ Advisory: https://rustsec.org/advisories/RUSTSEC-2024-0402
    ├ The borsh serialization of the HashMap did not follow the borsh specification.
      It potentially produced non-canonical encodings dependent on insertion order.
      It also did not perform canonicty checks on decoding.
      
      This can result in consensus splits and cause equivalent objects to be
      considered distinct.
      
      This was patched in 0.15.1.
```



<!--
Make sure the PR title and labels are set to maximize their usefulness
for the CHANGELOG,
and our `git log`.

If you have noticed any breaking changes, include them in the migration
guide.

We track various metrics at <https://build.rerun.io>.

For maintainers:
* To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
* To deploy documentation changes immediately after merging this PR, add
the `deploy docs` label.
-->
### What

After refactoring, _implicit_ nodes were visualized using regular
`text_color` and where therefore no possible to differentiate from
regular nodes. This PR fixes that.

<img width="44" alt="image"
src="https://github.com/user-attachments/assets/d1c32758-2154-4842-ab18-9be9b899c6d0">


The node on the bottom (darker gray) is implicit.

<!--
Make sure the PR title and labels are set to maximize their usefulness
for the CHANGELOG,
and our `git log`.

If you have noticed any breaking changes, include them in the migration
guide.

We track various metrics at <https://build.rerun.io>.

For maintainers:
* To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
* To deploy documentation changes immediately after merging this PR, add
the `deploy docs` label.
-->
@grtlr grtlr merged commit c0fad11 into main Dec 4, 2024
32 checks passed
@grtlr grtlr deleted the grtlr/butterfly-effect branch December 4, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat-graph-view Everything related to the graph view include in changelog ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carry over graph layout information between time points
4 participants