-
Notifications
You must be signed in to change notification settings - Fork 323
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
Opening Component Browser by clicking on the output port #3346
Opening Component Browser by clicking on the output port #3346
Conversation
…ement-by-dblclick-181076145
…ement-by-dblclick-181076145
#[derive(Clone, Copy, Debug)] | ||
#[derive(Clone, Debug)] |
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.
What is the reason to make this struct so heavyweight? Why do you need to keep EdgeEndpoint
instead of NodeId
, just like DroppingEdge
does?
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.
It was by my request. If we want to support multiple output ports (and we want eventually), then the future implementor would already know where to start.
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.
I am OK with adding the information. My question was why instead of adding simply NodeId
we are adding the whole structure there.
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.
Because it makes sense in this structure. The node was created by clicking on the port - the fact that we don't make use of this information right now is an implementation detail. And I would not be worried with weight of crumbs: those Vec
s are empty in most cases, and usually have no more than two elements otherwise.
app/gui/view/graph-editor/src/lib.rs
Outdated
@@ -2831,7 +2848,7 @@ fn new_graph_editor(app: &Application) -> GraphEditor { | |||
input_press : &node_input_touch.down, | |||
output : &out, | |||
}; | |||
model.create_node(&ctx, *way, *mouse_pos) | |||
model.create_node(&ctx, way.clone(), *mouse_pos) |
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.
Perhaps create_node
could take way
by a reference?
app/gui/view/graph-editor/src/lib.rs
Outdated
input_start_node_creation_from_port <- inputs.hover_node_output.sample(&inputs.start_node_creation_from_port); | ||
start_node_creation_from_port <- input_start_node_creation_from_port.filter_map(|v| v.clone()); | ||
start_creation_from_port_way <- start_node_creation_from_port.map( | ||
|endpoint| WayOfCreatingNode::StartCreationFromPortEvent{endpoint: endpoint.clone()}); | ||
removed_edges_on_node_creation_from_port <= start_creation_from_port_way.map(f_!(model.model.clear_all_detached_edges())); | ||
out.source.on_edge_drop <+ removed_edges_on_node_creation_from_port; |
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.
- Some of those lines are over 100 characters.
- In this section, FRP nodes are for just creating the
way
structure. Please move nodes specific to creating node from clicking output to a separate block.
/// Start creation of a new Node connected to the port that is currently under the cursor. | ||
/// If the cursor is currently not over any node's port, this event will have no effect. |
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.
I would repeat or refer to information in start_node_creation
that this is meant to be user interaction.
(Process note: the code-review is now Approved, therefore the PR is waiting for an Acceptance/QA Review on Task https://www.pivotaltracker.com/story/show/181076145 before merge.) |
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.
Accepted
Pull Request Description
Double-clicking a node's output port or clicking the port with a right mouse button (RMB) creates a new node aligned to the clicked node.
Visuals
The screencast below demonstrates the following features:
interface
demo scene).Screen.Recording.2022-03-17.at.17.02.58.mov
The supplementary screencast below demonstrates that double-clicking or RMB-clicking a node's output port cancels the action of dragging a new connection from a node.
Screen.Recording.2022-03-18.at.12.48.38.mov
https://www.pivotaltracker.com/story/show/181076145
Important Notes
cmd+g
keyboard shortcut after selecting a group of nodes). This PR keeps that functionality when the user double-clicks on a node, as long as the mouse is not positioned over the node's output ports.Crumb
) is passed into thecreate_node
function, but it is not passed further toNodeSource
. The Node Searcher currently does not support passing port information throughNodeSource
.Checklist
Please include the following checklist in your PR:
./run dist
and./run watch
.