Simplify graph viewer rendering logic #8227
Conversation
|
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
grtlr
changed the title
grtlr
changed the title
grtlr
force-pushed
the
grtlr/simplify-graph-viewer
branch
from
a800af0 to
22b5489
Compare
grtlr
force-pushed
the
grtlr/simplify-graph-viewer
branch
from
e0adeab to
2a1535c
Compare
grtlr
force-pushed
the
grtlr/simplify-graph-viewer
branch
from
3034d55 to
f50d698
Compare
| Explicit { | ||
| id: NodeId, | ||
| instance: Instance, | ||
| node: ArrowString, | ||
| position: Option<Pos2>, | ||
| label: DrawableLabel, | ||
| }, | ||
| Implicit { |
There was a problem hiding this comment.
Docstring these! I would not expect a reader to understand what an implicit node is (I don't)
There was a problem hiding this comment.
I've added doc comments, maybe this makes it clearer. Because Node::Implicit only has a very limited subset of information, I chose to introduce the enum at the top level to avoid any confusion. This is also the reason wy I opted against an enum NodeCategory { Explicit, Implicit }.
We could also introduce a mixture of both approaches, but that would introduce another level of indirection, which I think can be avoided by a little redundancy.
| nodes.push(Node::Implicit { | ||
| id: edge.source_index, | ||
| node: edge.source.0 .0.clone(), | ||
| label: DrawableLabel::implicit_circle(), | ||
| }); |
There was a problem hiding this comment.
Maybe it would make more sense with a enum NodeCategory { Implicit, Explicit } and struct Node { category: NodeCategory, id: … } ?
| // Sorry for the pun, could not resist 😎. | ||
| // On a serious note, is there no other way to create a `Sense` that does nothing? | ||
| const NON_SENSE: Sense = Sense { |
There was a problem hiding this comment.
Apparently not… PRs welcome :)
(Puns too)
There was a problem hiding this comment.
I've looked at the code, and apparently hover is equal to setting everything to false:
/// Senses no clicks or drags. Only senses mouse hover.
#[doc(alias = "none")]
#[inline]
pub fn hover() -> Self {
Self {
click: false,
drag: false,
focusable: false,
}
}There was a problem hiding this comment.
From the outside (not knowing the implementation details) it's a bit weird that egui does not have a hover: false here.
There was a problem hiding this comment.
re hover: false: all widgets in egui sense hover, i.e. can have a tooltip
| // For now we compute the entity rectangles on the fly. | ||
| let mut current_rect = egui::Rect::NOTHING; | ||
|
|
||
| for node in graph.nodes() { |
There was a problem hiding this comment.
This function is becoming both long and deep. Any chance we can break out one of the inner scopes to its own function?
There was a problem hiding this comment.
I will try to break it down a bit.
| } | ||
|
|
||
| /// Helper function to find the point where the line intersects the border of a rectangle | ||
| fn find_border_point(rect: Rect, direction: Vec2) -> Pos2 { |
There was a problem hiding this comment.
It would be nice with a unit-test for this.
Also: what is the line defined by the direction?
Btw, this is all pretty similar to https://docs.rs/emath/latest/src/emath/rect.rs.html#652-674 - maybe we can lift in the function into emath
There was a problem hiding this comment.
Done! If you like the code as it is, I'm happy to uptream to emath! I tried to give it a name similar to Rect::intersects_ray.
grtlr
added
the
exclude from changelog
Related
What
It changes two things:
egui::Areas, we now only have a single top-level area.Overall this will improve code quality, performance, and will unlock new GraphViz-like layout algorithms (which require defined label size from the beginning) down the road.