Node searcher zoom & edited node growth/shrink animation #3327

vitvakatu · 2022-03-08T09:28:14Z

Pull Request Description

In this PR two things are implemented:

Node Searcher zoom factor (and therefore its size) is fixed no matter how you move the main camera. The node searcher is also positioned directly below currently edited node at all times.
Node growth/shrink animation when you start/finish node editing. After animation end the edited node zoom factor is also fixed and matches the zoom factor of the node searcher.

See attached video with different ways of editing/creating nodes:

2022-03-09.03-23-24.mp4

Technical details

Added several additional scene layers for separate rendering: node_searcher, node_searcher_text, edited_node, edited_node_text. Searcher is always rendered by node_searcher camera, edited node moves between its usual layers and edited_node layer. Because text rendering uses different API, all node components were modified to support change of the layer.
Also added node_searcher DOM layer, because documentation is implemented as a DOM object.
Added two FRP endpoints for ensogl::Animation: on_end and set_value. These endpoints are useful while implementing growth/shrink animation.
Added FRP endpoints for the Camera2d: position and zoom outputs. This allows to synchronize cameras easily using FRP networks.
Growth/shrink animation implemented in GraphEditor by blending two animations, similar to Node Snapping implementation. However, shrinking animation is a bit tricky to implement correctly, as we must always return node back to the main scene layer after editing is done.

Important Notes

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the Scala, Java, and Rust style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

MichaelMauderer · 2022-03-10T10:32:53Z

app/gui/view/graph-editor/src/lib.rs

@@ -2596,6 +2612,59 @@ fn new_graph_editor(app: &Application) -> GraphEditor {
    }


+    // === Edited node growth/shrink animation ===


This logic should be outsourced to a separate file to avoid further growing the graph editor.

I agree, moved this FRP code to a separate module in component/node hierarchy (where I feel it belongs).

MichaelMauderer · 2022-03-10T10:36:34Z

app/gui/view/graph-editor/src/lib.rs

@@ -1428,6 +1430,20 @@ impl GraphEditorModelWithNetwork {
        node_id
    }

+    /// Move node to the `edited_node` scene layer, so that it is rendered by the separate camera.
+    pub fn move_node_to_edited_node_layer(&self, node_id: NodeId) {


Since this and the following method operate only on the nodes, they might be better suited to live in the Nodes struct directly.

These methods were pure helpers to avoid code repetition in FRP definition! We need access to GraphEditorModel because we need to access Nodes struct, and we can't implement growth/shrink animation inside component/node module, because it is implemented by moving a single edited_node camera, so this behavior can't be shared by all Nodes.

Now these functions are moved to a separate module.

app/gui/view/src/project.rs

wdanilo · 2022-03-11T04:16:44Z

Very nice results!!!!

This PR contains some heavy code, I need more time to review it – will be reviewed tomorrow.

wdanilo · 2022-03-14T10:15:01Z

CHANGELOG.md

@@ -2,6 +2,7 @@

 #### Visual Environment

+- [Node Searcher preserves its zoom factor.][3327]


This needs additional explanation. Thunk about changelog as something read by users. They should understand what happened after reading these notes. The link to the task is "additional technical explanation", but without seeing it, it should already be clear.

Added additional explanation, please take a look.

wdanilo · 2022-03-14T10:17:43Z

app/gui/view/graph-editor/src/component/node.rs

+    fn set_layers(&self, layer: &Layer, text_layer: &Layer, action_bar_layer: &Layer) {
+        layer.add_exclusive(&self.display_object);
+        action_bar_layer.add_exclusive(&self.action_bar);
+        self.output.set_label_layer(text_layer);
+        self.input.set_label_layer(text_layer);
+        self.profiling_label.set_label_layer(text_layer);
+        self.comment.add_to_scene_layer(text_layer);
+    }
+
+    /// Move all sub-components to `edited_node` layer.
+    ///
+    /// A simple [`Layer::add_exclusive`] wouldn't work because text rendering in ensogl uses a
+    /// separate layer management API.
+    pub fn move_to_edited_node_layer(&self) {
+        let scene = &self.app.display.default_scene;
+        let layer = &scene.layers.edited_node;
+        let text_layer = &scene.layers.edited_node_text;
+        let action_bar_layer = &scene.layers.edited_node;
+        self.set_layers(layer, text_layer, action_bar_layer);
+    }
+
+    /// Move all sub-components to `main` layer.
+    ///
+    /// A simple [`Layer::add_exclusive`] wouldn't work because text rendering in ensogl uses a
+    /// separate layer management API.
+    pub fn move_to_main_layer(&self) {
+        let scene = &self.app.display.default_scene;
+        let layer = &scene.layers.main;
+        let text_layer = &scene.layers.label;
+        let action_bar_layer = &scene.layers.above_nodes;
+        self.set_layers(layer, text_layer, action_bar_layer);


this needs additional explanation in the documentation why action_bar_layer is handled in such a special way. This is not obvious from reading the code, so I'd be very thankful for making the docs a little bit more expanded here with clear explanation of which layer handles what in every scenario.

action_bar is the only node component that uses a separate scene layer (above_nodes). I'm not aware of the exact reasons behind that. I added a note to the docs about that.

wdanilo · 2022-03-14T10:22:14Z

app/gui/view/graph-editor/src/component/node/growth_animation.rs

+        let main_cam_frp = main_cam.frp();
+
+
+        // === Starting  node editing. ===


double space + we are not using dots in section titles - in English, section titles are dotless sentences. Which is kind of stupid, but it is this way.

wdanilo · 2022-03-14T10:28:29Z

app/gui/view/graph-editor/src/component/node/growth_animation.rs

+        _eval <- all_with(&out.node_editing_started, &previous_edited_node, f!([model] (current, previous) {
+            move_node_to_main_layer(&model, *previous);
+            move_node_to_edited_node_layer(&model, *current);
+        }));


I don't like these functions that take model as argument, as they clearly belong to model. This can be rewritten to:

_eval <- all_with(&previous_edited_node, &out.node_editing_started, f!((previous, current) { model.move_node_to_main_layer(previous); model.move_node_to_edited_node_layer(current); }));

I know that model is in another module, but you can just extend it with a local trait in this module.

It was written that way, but after moving to a separate module I changed methods to functions. Fixed it back now.

wdanilo · 2022-03-14T10:30:14Z

app/gui/view/graph-editor/src/component/node/growth_animation.rs

+            let weight = Vector3::from_element(*weight);
+            let inv_weight = Vector3::from_element(1.0) - weight;
+            target.component_mul(&weight) + animation.component_mul(&inv_weight)
+        });


Can we format such lines this way instead, please?

edited_node_cam_position <- all_with3 (&edited_node_cam_target, &growth_animation.value, &animation_blending.value, |target,animation,weight| { let weight = Vector3::from_element(*weight); let inv_weight = Vector3::from_element(1.0) - weight; target.component_mul(&weight) + animation.component_mul(&inv_weight) });

wdanilo · 2022-03-14T10:34:46Z

app/gui/view/src/project.rs

+                let move_to_edited_node = searcher_pos * (1.0 - zoom);
+                preserve_zoom + move_to_edited_node
+            });
+            eval searcher_cam_pos ([searcher_cam] (pos) searcher_cam.set_position_xy(*pos));


you don't need this part here [searcher_cam]. If the expression starts with foo.<something>, then foo will automatically be threaded as clone able thing by the f! macro (and the eval construct uses f! under the hood).

Good to know, fixed!

wdanilo · 2022-03-14T10:35:19Z

lib/rust/ensogl/core/src/animation/frp/animation.rs

-//         on_end (inertia::EndStatus),
+//         on_end (),
 //     }
 // }


this is an edit in a commented code. Is this on purpose? Why this code is commented in the first place?

Removed this code. I suppose it lives here since prehistoric times.

wdanilo · 2022-03-14T10:35:45Z

lib/rust/ensogl/core/src/animation/frp/animation.rs

+/// Default animation precision.
+pub const DEFAULT_PRECISION: f32 = 0.001;
+


This doc does not explain what it is at all. After reading it, I still dont understand what it does.

Added additional docs

wdanilo · 2022-03-14T10:37:35Z

lib/rust/ensogl/core/src/animation/frp/animation.rs

+            set_value <- any_mut::<T>();
            precision <- any_mut::<f32>();
            skip      <- any_mut::<()>();
            eval  target    ((t) simulator.set_target_value(mix::into_space(t.clone())));
+            eval  set_value ((t) simulator.set_value(mix::into_space(t.clone())));


I don't like the idea for the ability to set the value of the physics-based animator. It can make the animations jumpy. It was not exposed on purpose. Can we have a fast call why this is needed and if there is a way not to expose it, please?

Oh well, actually it turned out that I'm not using it anywhere :) It was used in some previous versions of the code. Removed it

lib/rust/ensogl/core/src/display/scene.rs

app/gui/view/src/project.rs

lib/rust/ensogl/core/src/animation/frp/animation.rs

vitvakatu self-assigned this Mar 8, 2022

vitvakatu force-pushed the wip/vitvakatu/searcher-zoom-181181221 branch from 86ef3f5 to 5c41a50 Compare March 8, 2022 10:35

vitvakatu added 6 commits March 8, 2022 15:54

Reverting commit 0836ce7

2d0420c

Merge branch 'develop' into wip/vitvakatu/revert-adding-node

284211b

fix clippy warnings about missing docs

bdcdeca

Implement edited node growth/shrink animation and fixed searcher zoom

aa3eaa1

fixes after merge

21bc3b6

Additional fixes

43c2920

vitvakatu force-pushed the wip/vitvakatu/searcher-zoom-181181221 branch from 5c41a50 to 43c2920 Compare March 8, 2022 23:50

vitvakatu added 3 commits March 9, 2022 03:14

self-review, expand docs

72c848d

Merge branch 'develop' into wip/vitvakatu/searcher-zoom-181181221

d2a1f28

slightly adjust formatting

4d62004

vitvakatu marked this pull request as ready for review March 9, 2022 12:07

vitvakatu requested review from MichaelMauderer, wdanilo and farmaazon as code owners March 9, 2022 12:07

update changelog

dfae133

MichaelMauderer reviewed Mar 10, 2022

View reviewed changes

vitvakatu added 2 commits March 14, 2022 12:52

Refactor growth/shrink animation to a separate module

930400a

Merge branch 'develop' into wip/vitvakatu/searcher-zoom-181181221

0dd3bac

vitvakatu requested a review from MichaelMauderer March 14, 2022 10:00

wdanilo requested changes Mar 14, 2022

View reviewed changes

Various review fixes

7db923a

vitvakatu requested a review from wdanilo March 14, 2022 13:56

vitvakatu added 2 commits March 16, 2022 00:15

Simplify shrinking animation implementation

437758a

Merge branch 'develop' into wip/vitvakatu/searcher-zoom-181181221

9673a0e

vitvakatu requested review from wdanilo and removed request for MichaelMauderer and wdanilo March 16, 2022 15:08

vitvakatu requested a review from MichaelMauderer March 16, 2022 15:08

vitvakatu added 2 commits March 17, 2022 00:55

Merge branch 'develop' into wip/vitvakatu/searcher-zoom-181181221

2ee7236

Implement flexible animation duration

68a9470

wdanilo approved these changes Mar 17, 2022

View reviewed changes

app/gui/view/src/project.rs Outdated Show resolved Hide resolved

lib/rust/ensogl/core/src/animation/frp/animation.rs Outdated Show resolved Hide resolved

Fix formatting

1bc10c6

4e6 approved these changes Mar 17, 2022

View reviewed changes

4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Mar 17, 2022

mergify bot merged commit cdcc852 into develop Mar 17, 2022

mergify bot deleted the wip/vitvakatu/searcher-zoom-181181221 branch March 17, 2022 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node searcher zoom & edited node growth/shrink animation #3327

Node searcher zoom & edited node growth/shrink animation #3327

vitvakatu commented Mar 8, 2022 •

edited

Loading

MichaelMauderer Mar 10, 2022

vitvakatu Mar 14, 2022

MichaelMauderer Mar 10, 2022

vitvakatu Mar 14, 2022

wdanilo commented Mar 11, 2022

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022 •

edited

Loading

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022

wdanilo Mar 14, 2022

vitvakatu Mar 14, 2022 •

edited

Loading

		@@ -2596,6 +2612,59 @@ fn new_graph_editor(app: &Application) -> GraphEditor {
		}


		// === Edited node growth/shrink animation ===

		@@ -2,6 +2,7 @@

		#### Visual Environment

		- [Node Searcher preserves its zoom factor.][3327]

		let main_cam_frp = main_cam.frp();


		// === Starting node editing. ===

		/// Default animation precision.
		pub const DEFAULT_PRECISION: f32 = 0.001;

Node searcher zoom & edited node growth/shrink animation #3327

Node searcher zoom & edited node growth/shrink animation #3327

Conversation

vitvakatu commented Mar 8, 2022 • edited Loading

Pull Request Description

Technical details

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdanilo commented Mar 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitvakatu Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitvakatu Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

vitvakatu commented Mar 8, 2022 •

edited

Loading

vitvakatu Mar 14, 2022 •

edited

Loading

vitvakatu Mar 14, 2022 •

edited

Loading