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.

Sign up for GitHub

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

Jump to bottom

Fix visualisation FRP bugs. #6831

Merged

mergify merged 11 commits into develop from wip/MichaelMauderer/Visualisation_FRP_Refactoring_And_Bugfixes

Jun 5, 2023

Contributor

MichaelMauderer commented May 24, 2023 •

edited

Loading

Pull Request Description

Fixes

Empty Visualization when opening a full-screen visualization directly without opening the visualization before. Full-screen visualization is blank when opening it for the first time #6770

Peek.2023-05-22.15-17.mp4

Empty visualization when opening the full-screen visualization before the data for the visualization has arrived. Visualizations updates are not stable and leaving visualizations blank #6561

Peek.2023-05-25.12-52.mp4

Visualization is reset to default when reconnecting nodes Visualization is reset to default when reconnecting nodes #6673

Peek.2023-05-25.12-55.mp4

Redundant internal open/lose events caused by logic loops around the show/hide button, as well as many redundant layer setting/unsetting issues internal to the visualization code.

Generally improves the logic around the visualization API by avoiding decentralized logic in different places and removing old code that is no longer needed.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

MichaelMauderer self-assigned this

MichaelMauderer force-pushed the wip/MichaelMauderer/Visualisation_FRP_Refactoring_And_Bugfixes branch from 411b592 to 2846934 Compare

May 24, 2023 13:06


          Refactor FRP for visualisation to fix bugs described in #6673, #6561 …

ea162d3

…as well as some other undiscovered issues around the order of events that arrive at the visualisation container.

MichaelMauderer force-pushed the wip/MichaelMauderer/Visualisation_FRP_Refactoring_And_Bugfixes branch from 2846934 to ea162d3 Compare

May 25, 2023 11:58

MichaelMauderer added the CI: No changelog needed label

MichaelMauderer marked this pull request as ready for review

May 25, 2023 11:58

MichaelMauderer requested review from wdanilo, farmaazon, kazcw and mwu-tow as code owners

May 25, 2023 11:58

This was linked to issues May 25, 2023

Visualizations updates are not stable and leaving visualizations blank #6561

Closed

Full-screen visualization is blank when opening it for the first time #6770

Closed

kazcw approved these changes

View reviewed changes

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated

+              /// Indicates the visibility state of the visualisation.
+              #[derive(Clone, Copy, Debug, PartialEq)]
+              pub enum ViewState {
+                  /// Visualisation is permanently enabled and visible in thw graph editor. It is attached to a

Contributor

kazcw May 25, 2023

Suggested change

      
                /// Visualisation is permanently enabled and visible in thw graph editor. It is attached to a
          
                /// Visualisation is permanently enabled and visible in the graph editor. It is attached to a

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated

Comment on lines 153 to 157

+              impl Default for ViewState {
+                  fn default() -> Self {
+                      ViewState::Disabled
+                  }
+              }

Contributor

kazcw May 25, 2023

Derivable.

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated

Comment on lines 162 to 169

+                  pub fn visible(&self) -> bool {
+                      !matches!(self, ViewState::Disabled)
+                  }
+                  /// Indicates whether the visualisation is fullscreen mode.
+                  pub fn is_fullscreen(&self) -> bool {
+                      matches!(self, ViewState::Fullscreen)
+                  }

Contributor

kazcw May 25, 2023

Inconsistent is_

farmaazon approved these changes

View reviewed changes

Contributor

farmaazon left a comment

The code looks good, with some comments below. But I'm worried we have regressions in displaying errors (the visualization should be hidden on error). Should be checked during QA.

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated

Comment on lines 2 to 3

		//! a visualisation int he graph editor and includes a visual box that contains the visualisaiton,
		//! a action bar that allows setting the visualisation type.

Contributor

farmaazon May 25, 2023

Suggested change

      
            //! a visualisation int he graph editor and includes a visual box that contains the visualisaiton,
          
            //! a action bar that allows setting the visualisation type.
          
            //! a visualisation in the graph editor and includes a visual box that contains the visualisaiton,
          
            //! and the action bar that allows setting the visualisation type.

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated

Comment on lines 7 to 8

		//! is correctly positioned, sized and layouted in its different `ViewState`s (which include the
		//! `Enabled`, `Fullscreen` and `Preview` states). Importantly, this also includes layer management

Contributor

farmaazon May 25, 2023

You can put ViewState in [] ([ViewState]) making them link to the structure; then I think it's not necessary to list enum variants.

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated

Comment on lines 8 to 9

		//! `Enabled`, `Fullscreen` and `Preview` states). Importantly, this also includes layer management
		//! and ensuring that the visualisation is correctly layered with respect to other scene objects.

Contributor

farmaazon May 25, 2023

I don't quite understand the "layer management". Does visualization create new layers? What "correct layering" means? Is visualization split into layers?

Contributor Author

MichaelMauderer May 29, 2023

This is about EnsoGL layers and management of occlusion. I clarified this now.

app/gui/view/graph-editor/src/component/visualization/container.rs

+                  Enabled,
+                  /// Visualisation is disabled and hidden in the graph editor.
+                  Disabled,
+                  /// Visualisation is temporarily enabled and visible in the graph editor. It should be placed

Contributor

farmaazon May 25, 2023

Is any difference between "enabled" and "visible"? Before, "enabled" meant that the visualization "eye" was toggled on. Visualization could be visible and not enabled (in case of Preview) and enabled but not visible (if there's error on the node - I think we miss that variant here).

Contributor Author

MichaelMauderer May 29, 2023

This logic is now decoupled; thus, the visualization is either visible or not. While the button's state is managed independently. I will double-check the button has the correct state during preview.

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated

                               Some(visualization::Registry::default_visualisation())
                           });
-                          vis_input_type <- frp.set_vis_input_type.on_change();
+                          vis_input_type <- input.set_vis_input_type.on_change();
                           vis_input_type <- vis_input_type.gate(&visualisation_uninitialised).unwrap();

Contributor

farmaazon May 25, 2023

I know it's an older code, but the name is bad, as it says nothing about the type being updated only when visualization is uninitialized.

Contributor Author

MichaelMauderer May 29, 2023

I've updated the naming. But it does get a bit verbose.

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated

Comment on lines 573 to 574

		visualisation_uninitialised <- input.set_visualization.map(\|t\| t.is_none());
		input_type_uninitialised <- input.set_vis_input_type.is_some().not();

Contributor

farmaazon May 25, 2023

The name "visualization_uninitialized" is a bit unfortunate. I think "non-chosen" says better what it is.

Contributor Author

MichaelMauderer May 29, 2023

I've updated the naming.

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated Show resolved Hide resolved

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated Show resolved Hide resolved

app/gui/view/graph-editor/src/component/visualization/container.rs Outdated Show resolved Hide resolved

app/gui/view/graph-editor/src/component/visualization/container/action_bar.rs Outdated

Comment on lines 394 to 396

+                          has_selection <- visualization_chooser.chosen_entry.is_some();
+                          frp.source.visualisation_selection
+                              <+ visualization_chooser.chosen_entry.gate(&has_selection);

Contributor

farmaazon May 25, 2023

.filter_map(|entry| *entry) does it in one line. And I use it a lot - maybe we could make filter_some node?

Contributor

Frizi May 30, 2023

I think .unwrap() already does exactly that.

wdanilo approved these changes

View reviewed changes

Member

wdanilo left a comment

Amazing that it's here! Skipping my code review seeing code reviews of Kaz and Adam :)

MichaelMauderer mentioned this pull request

Some visualisations do not load on project start #6766

Closed

2 tasks

MichaelMauderer and others added 7 commits

May 29, 2023 12:22


          Update app/gui/view/graph-editor/src/component/visualization/containe…

ccc0806

…r.rs

Co-authored-by: Adam Obuchowicz <[email protected]>


          Update app/gui/view/graph-editor/src/component/visualization/containe…

c447717

…r.rs

Co-authored-by: Adam Obuchowicz <[email protected]>


          Update app/gui/view/graph-editor/src/component/visualization/containe…

7f4f963

…r.rs

Co-authored-by: Adam Obuchowicz <[email protected]>


          Review feedback.

dd42f40


          Merge branch 'develop' into wip/MichaelMauderer/Visualisation_FRP_Ref…

0b606c3

…actoring_And_Bugfixes

# Conflicts:
#	app/gui/view/graph-editor/src/component/visualization/container.rs


          Fix merge issues.

2051aad


          Fmt.

04771b2

Contributor

vitvakatu commented May 31, 2023

QA Report 🔴

Most importantly, error visualization is indeed affected, as @farmaazon noticed in his review.

Hovering the output with the mouse displays both the error message and a visualization. Switching visualization with the icon doesn't work, but using space or shift-space shortcuts toggles it.

2023-05-31.12-36-42.mp4

Also, I noticed that on complex projects a lot of visualizations disappear after loading. For example, for COVID project on my machine:

2023-05-31.12-31-51.mp4


          Fix error visualisation behaviour.

Contributor Author

MichaelMauderer commented Jun 1, 2023 •

edited

Loading

Most importantly, error visualization is indeed affected, as @farmaazon noticed in his review.

This should now be fixed.

Also, I noticed that on complex projects a lot of visualizations disappear after loading. For example, for COVID project on my machine:

I can reproduce this also on develop and this seems to be #6724.

Contributor

vitvakatu commented Jun 2, 2023

@MichaelMauderer shift-tab hotkey still opens fullscreen vis on a node with error. Is this intentional?


          Correctly handle fullscreen mode for visualisation with error.

37d1f5a

Contributor Author

MichaelMauderer commented Jun 2, 2023

@MichaelMauderer shift-tab hotkey still opens fullscreen vis on a node with error. Is this intentional?

I think that does not make sense, even if it works like that on develop. So, it is fixed now to stop the empty vis from opening.

Contributor

vitvakatu commented Jun 2, 2023

QA: passed 🟢

MichaelMauderer added CI: Ready to merge CI: Keep up to date and removed CI: Keep up to date labels


          Bump wasm size.

b8cb70e

MichaelMauderer mentioned this pull request

Issue with undo and showing/hiding visualizations. #6589

Closed

mergify bot merged commit 72b202b into develop

mergify bot deleted the wip/MichaelMauderer/Visualisation_FRP_Refactoring_And_Bugfixes branch

June 5, 2023 16:01

hubertp mentioned this pull request

After opening any project, the types under nodes are not displayed when hovered #6994

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Frizi Frizi left review comments

kazcw kazcw approved these changes

wdanilo wdanilo approved these changes

farmaazon farmaazon approved these changes

mwu-tow Awaiting requested review from mwu-tow

Labels

CI: No changelog needed CI: Ready to merge