Allow dragging nodes to block canvas #227
Conversation
Back in a808f67 and 43dd08b, the editor selection was dynamically updated so that both the BlockCode node and its parent were selected. This would open the parent node in the 2D/3D scene editor while the BlockCode node was open in the block canvas. However, selecting both nodes at the same time puts the editor into a multi-selection mode that makes interaction with the scene tree dock awkward. For example, clicking on another node does not select it for dragging. Furthermore, since some parts of the code operate on the selection while others wait to see the inspector's edited object is, there can be strange interactions when the selection is changed in the middle of an input action. What we really want is a way to say "make this the active scene in the scene editor" without selecting the node like we can with BlockCode nodes and the block canvas. That doesn't appear possible with Godot, so just always look at what the currently edited object in the inspector is and stop trying to activate the BlockCode parent in the scene editor. https://phabricator.endlessm.com/T35648
Depending on where the two nodes are in a scene, the path to the target node may be better represented as relative or absolute. Add the node_scene_path helper function to make that determination consistently. https://phabricator.endlessm.com/T35648
manuq
left a comment
manuq
left a comment
There was a problem hiding this comment.
It works like a charm! Thanks @dylanmccall to make this so easy. I have only added a fixup commit to pass the tests, and added a suggestion to remove the repeated "node" word.
Add a block to get a node from a path. A value block returning an Object is used. An extension script is used to query all the nodes in the scene and provide them as options. https://phabricator.endlessm.com/T35648
When a node is dragged from the scene tree dock, dynamically create a get_node block for it and add it to the canvas. This is an alternative to using the get_node block from the picker and changing the drop down to the appropriate path. https://phabricator.endlessm.com/T35648
dbnicholson
force-pushed
the
T35648-node-drag
branch
from
122aa19 to
2714dde
Compare
|
One note about the multi-select behaviour: it was also so, when someone is editing a block code node, the node it belongs to is highlighted in the canvas. I think it's fine losing that since it didn't work super well anyway and this feature is far more important, but it was something that was casually requested at some point. A better approach would be actually adding a representation of BlockCode itself in the 2D / 3D canvas, but that would have to be pretty clever since it would depend on what type the parent node is. |
| ## Both [param node] and [param reference] must be ancestors of [param | ||
| ## path_root]. If [param path_root] is [constant null] | ||
| ## [method EditorInterface.get_edited_scene_root().get_parent] is used. | ||
| static func node_scene_path(node: Node, reference: Node, path_root: Node = null) -> NodePath: |
There was a problem hiding this comment.
This is great. I wonder if you had to read the editor source for reference or implemented it from scratch.
There was a problem hiding this comment.
It was a lot of trial and error and printing out paths until I felt reasonably comfortable that I knew the pattern. I also had to read the SceneTree, NodePath and Node documentation many times. In particular, I didn't understand that get_edited_scene_root() or SceneTree.edited_scene_root don't correspond to /root. /root corresponds to the parent of the scene root node. That's SceneTree.root, but in the editor that's the entire editor window. Hence why it does get_edited_scene_root().get_parent() since you're deep in the editor hierarchy in our cases.
I'm sure there are some corner cases that aren't covered, but I think it will fit our needs.
There was a problem hiding this comment.
Yeah this is certainly something to improve upstream. Why dragging a node from the current scene tree would give you a path relative to the editor root?
There was a problem hiding this comment.
Because the current scene is the whole editor. The path you're given is an absolute path that's entirely valid while the editor is running because that's where it is in the (singleton) SceneTree.
I should have put this in a comment, but this is a special case for us. We're trying to create an absolute path for when edited_scene_root later becomes current_scene. The absolute '/root` path returned from here is bogus while you're in the editor. It only becomes valid when the scene is actually running. That's what we need here because we're generating a script that won't actually run until the edited scene becomes the current scene and is at the root of the scene tree.
If you just want an absolute path to the node in whatever is the current scene, you can just use Node.get_path(). Originally I wrote this to always use relative paths as those work no matter where the scene is in the scene tree. That's also why you use relative paths such as $Foo/Bar (equivalent to get_node("Foo/Bar")) in GDScript - they work regardless of where the node using the script is in the scene tree. However, relative paths with ../../SomeNode seemed too hard to grasp, which is why I came up with the "absolute path that will become valid at runtime" thing.
So, if you wanted to upstream this, it would have to be a separate method like get_edited_path(relative: bool = false) that returns a path treating the edited scene as if it was the current scene.
Ah yes, that would be much welcome. A way to highlight the target node. I think that can be done with an editor plugin for any node that inherits CanvasItem by using get_viewport_rect() to get the bounding box. |
| var block = _context.block_script.instantiate_block_by_name(&"get_node") | ||
| block.set_parameter_values_on_ready({"path": node_path}) | ||
| add_block(block, at_position) | ||
| reconnect_block.emit(block) |
There was a problem hiding this comment.
I have a suggestion for some future work here. Instead of setting the value of an option parameter in the get_node block, we could use the same scheme as parameter blocks, so in this spot we'd have something like instantiate_block_by_name("get_node:${node_path}") and it would generate a unique parameter block just for that node path (instead of a block with a big options list and the value set to the node path).
The hard part would be fixing the inevitable small issues which arise from doing that, since right now we only use the block_name:detail syntax for the one thing, but this use case is definitely something I had in mind when I added it :)
(But I don't mind at all that you did it this way. I like how we're stretching existing functionality here).
There was a problem hiding this comment.
I definitely considered using a unique block here and adding (or in this case, reusing) some method of getting it back when deserializing. I ended up deciding that it did want it to be like a get_node block that you got from the picker where you could change the path, but I don't feel that strongly about it either way. I think there would be value in having a specific block when you drop a specific node.
In order to call methods on other nodes, you need a reference to the node. This adds a new
get_nodeblock definition that has all the other nodes provided as options. It also adds a method to drag a node from the scene tree dock and set the node's path in aget_nodeblock automatically.Unfortunately, in order to make the drag action usable, I had to drop the multi-select feature that automatically activates the block node's parent in the scene editor. I searched long for a way to do that without selecting the node, but it doesn't appear that there's a way to do it.
https://phabricator.endlessm.com/T35648