-
Notifications
You must be signed in to change notification settings - Fork 321
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
Resizing visualizations #7164
Resizing visualizations #7164
Conversation
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.
If adding new layers is a way to go, agreed with @wdanilo, then it's ok, but otherwise I would restrain from adding new layers.
/// Default width and height of the visualization container. | ||
pub const DEFAULT_SIZE: (f32, f32) = (200.0, 200.0); | ||
/// Minimal allowed size of the visualization container. | ||
const MIN_SIZE: Vector2 = Vector2(200.0, 200.0); |
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.
The DEFAULT_SIZE
could also have Vector2 type.
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.
Done
@@ -278,11 +282,15 @@ impl View { | |||
} | |||
|
|||
fn init(self) -> Self { | |||
self.init_background(); | |||
let styles = StyleWatch::new(&self.scene.style_sheet); |
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.
The FIXME
comment was lost. You can also take the chance and change it to StyleWatchFRP (but it would need some FRP nodes for refreshing).
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.
Restored the fixme comment, also used FRP for part of the stuff in this method. The main stopper is shadow for dom, I don't think we have FRP-based API for that at the moment.
@@ -561,13 +587,18 @@ impl Container { | |||
let action_bar = &model.action_bar.frp; | |||
let registry = &model.registry; | |||
let selection = Animation::new(network); | |||
let width_anim = Animation::new(network); | |||
|
|||
let on_down = model.view.resize_grip.on_event_capturing::<mouse::Down>(); |
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.
Why do we need to do this on capturing phase? Shouldn't the grip be just over elements?
Also, the var name does not say it's resizing mouse down, so looking 30 lines below we may miss this information.
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.
TBH I just copied bits if @wdanilo's implementation of drag-n-drop in the list editor :D
Removed capturing, seems to be working just fine.
let width_anim = Animation::new(network); | ||
|
||
let on_down = model.view.resize_grip.on_event_capturing::<mouse::Down>(); | ||
let on_up_source = scene.on_event::<mouse::Up>(); |
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.
source
suggests this is a source node. Why does this need the prefix, and on_down
does not?
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.
Removed it
let overlay = Rectangle::default().build(|r| { | ||
r.set_color(INVISIBLE_HOVER_COLOR).set_border_color(INVISIBLE_HOVER_COLOR); | ||
}); | ||
let resize_grip = Rectangle::default().build(|r| { | ||
r.set_color(INVISIBLE_HOVER_COLOR).set_border_color(INVISIBLE_HOVER_COLOR); | ||
}); |
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.
Looking through the code, I have no idea how it works: we have only one rectangle for resize grip, which somehow covers both axes, while being also a child of overlay (so technically, it's over overlay).
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.
viz_resize_grip, | ||
viz_overlay, |
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.
If I understand EnsoGL correctly, we do not have any actual performance gains from using Rectangle if it forces us to create new layers, as the layers are a separate draw calls anyway. Perhaps the resize_grip could be a custom shape? That would make the code simpler (we already have a bit too many layers).
If you add new layers because you need to set ordering of the rectangles, you should probably use instance ordering implemented in #6140 instead.
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'm not using layers here, but rather partition layers implemented in #6140, as you suggested. All these added layers are "fake" ones, so everything should be ok here.
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.
Ah, I forgot about partition layers (and that they are in the same place as normal layers).
After discussion with @Frizi I decided to take a second look at the selection shape, and was able to replace it with a Rectangle. So one less custom shader is used in the application now. In total, this PR replaces two custom shapes with Rectangles. I decided to include this change along with review fixes into this PR. @farmaazon you can review this comment: f290006 I also checked how table visualization looks like on develop – and it doesn't have rounded corners there either, so I'm removing mentioning it from the PR description. |
As discussed on the demo, I added max size restriction (80% of screen dimensions) and disabled size reset when opening the visualization. Both options can be adjusted by code constants. |
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
@vitvakatu would it be possible to remove animation when dragging the size with mouse? If its hard, skip my comment. |
is that still true after yesterday's meeting? |
Fixed.
There is no animation when dragging the size with mouse. Animation is only applied when the node size changes. |
QA report 🟡No regressions, but resizing some visualizations causes a significant performance drop during resize. resize-test-2023-07-04_11.54.34.mp4It's probably caused by reflow. @Frizi @wdanilo do you have any ideas how to fix that? Probably we could update HTML only when user stops dragging, but that may worse UX as well (sometimes I want to see the resizing "live" so I know when it's enough). |
Merging after discussion, we can address performance issues in the future if needed. |
Pull Request Description
Closes #7047
Adds an ability to resize visualizations by dragging a special (invisible) shape along the bottom and right borders of visualizations.
2023-07-03.13-18-16.mp4
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.