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

Add support for Bezier-curve multi (self-)edges #8256

Merged
merged 13 commits into from
Dec 3, 2024
Merged

Conversation

grtlr
Copy link
Contributor

@grtlr grtlr commented Nov 29, 2024

Related

What

This PR brings self-edges in the form of Bezier-curves.

image

It also tries to make multiple edges between the same node more legible by applying a small arc on such edges to prevent overlap.

Copy link

github-actions bot commented Nov 29, 2024

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

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

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

@grtlr grtlr force-pushed the grtlr/graph-better-edges branch from e194dc2 to c550b56 Compare December 2, 2024 08:35
@grtlr grtlr changed the title Add support for Bezier-curve self-edges Add support for Bezier-curve multi (self-)edges Dec 2, 2024
@grtlr grtlr added 📺 re_viewer affects re_viewer itself ui concerns graphical user interface and removed 📺 re_viewer affects re_viewer itself labels Dec 2, 2024
@grtlr grtlr marked this pull request as ready for review December 2, 2024 11:18
@abey79 abey79 self-requested a review December 3, 2024 08:48
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.

Very nice!

tests/python/release_checklist/check_graph_view.py Outdated Show resolved Hide resolved
crates/viewer/re_space_view_graph/src/layout/request.rs Outdated Show resolved Hide resolved
/// Represents a cubic bezier curve.
///
/// In the future we could probably support more complex splines.
CubicBezier {
Copy link
Member

Choose a reason for hiding this comment

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

Note: we already depends on kurbo. I have a good experience using this crate for geometric primitives. Might be worth using it for some computations (e.g. bbox, linear approximation, etc.) if the need increases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I heard good things about kurbo too!

Copy link
Member

Choose a reason for hiding this comment

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

I've been using it lots in my personal projects.

Line { source, target } => (source.to_vec2() - target.to_vec2()).normalized(),
CubicBezier {
source, control, ..
} => (control[0].to_vec2() - source.to_vec2()).normalized(),
Copy link
Member

Choose a reason for hiding this comment

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

e.g. kurbo could provide the actual tangent here

crates/viewer/re_space_view_graph/src/layout/provider.rs Outdated Show resolved Hide resolved
target: anchor + Vec2::RIGHT * 4.,
// TODO(grtlr): The actual length of that spline should follow the `distance` parameter of the link force.
control: [
anchor + Vec2::new(-30. * offset, -40. * offset),
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that self edge are always above a node? This feels like a limitation. Ideally, the orientation of the self-edge(s) would also be set by the layout engine (e.g. based on the user provided position if any).

image

Probably not for this PR, but maybe worth having an issue and a TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry I should have mentioned. I didn't want to get into edge routing just yet. I've created an issue here to track this: #8291

points: [source, control[0], control[1], target],
closed: false,
fill: Color32::TRANSPARENT,
stroke: stroke.into(),
Copy link
Member

Choose a reason for hiding this comment

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

oh nice, that was easy enough :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we will run into limitations though when we want to draw more complex edges (such as in GraphViz layouts), because that's currently not supported by egui.

Copy link
Member

Choose a reason for hiding this comment

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

This is where kurbo can help. It can convert various things to cubic bezier, and can also linearise everything to polylines.

}

// We can add interactions in the future, for now we simply allocate the
// rect, so that bounding boxes are computed correctly.
ui.allocate_rect(rect, NON_SENSE)
ui.allocate_rect(geometry.bounding_rect(), NON_SENSE)
Copy link
Member

Choose a reason for hiding this comment

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

idiomatically, NON_SENSE is egui::Sense::hover(). Also, nonsense sounds... weird :)

Copy link
Contributor Author

@grtlr grtlr Dec 3, 2024

Choose a reason for hiding this comment

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

That was a pun that somehow made it through the previous review process, I've removed the code 😇.

@grtlr grtlr merged commit 0e9d221 into main Dec 3, 2024
32 checks passed
@grtlr grtlr deleted the grtlr/graph-better-edges branch December 3, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle self-edges, duplicated nodes, and duplicated edges in graph viewer
2 participants