update panels and their scrollbars when window resizes #358

michaelkirk · 2020-10-01T23:50:53Z

Panels calculate their container size and whether or not to show scroll bars based on the canvas size. They do this only once when the panel is first built.

But canvas size changes as window size changes.

Window size can change manually as the user manually adjusts it, or plugs in an external monitor, etc. But one place this is particularly annoying for me personally is upon first launch. Upon launch the window goes through a series of rapid size changes, starting small and growing to full size (this might just be a macos thing, I'm not sure?).

But this means the panels are all drawn WRT to the initial small size.

before

e.g. look how the top center panel looks broken here:

But even putting the initial launch thing aside, there are a bunch of expected scenarios where the use can change their window size mid-run, and we should try to handle them.

This PR moves the scrollbar manipulation into the Panel, rather than the PanelBuilder, so that it can be recomputed as necessary.

after

michaelkirk · 2020-10-01T23:51:28Z

widgetry/src/widgets/mod.rs

@@ -630,7 +630,7 @@ impl Widget {
                w.restore(ctx, prev);
            }
        } else if self.widget.can_restore() {
-            if let Some(ref other) = prev.top_level.find(self.id.as_ref().unwrap()) {
+            if let Some(ref other) = prev.maybe_find(self.id.as_ref().unwrap()) {


I wanted to make top_level private so I could confidently mess with it within Panel, this was the only external usage.

michaelkirk · 2020-10-01T23:53:31Z

widgetry/src/widgets/panel.rs

+        // Wrap the main widget in scrollable containers if necessary.
+        if self.scrollable_x && !old_scrollable_x {
+            let mut place_holder = Widget::nothing();
+            std::mem::swap(&mut place_holder, &mut self.top_level);


My Rust-fu is weak. Is there a better way to do this shuffling?

The old self.top_level is owned by the new self.top_level so there's kind of an ownership circle without temporarily introducing this Widget::nothing()

You can do let placeholder = std::mem::replace(&mut self.top_level, Widget::nothing()). That's a slightly shorter form, equivalent to what you have here

That is a little better - updated!

michaelkirk · 2020-10-01T23:56:21Z

widgetry/src/widgets/panel.rs

@@ -180,10 +262,9 @@ impl Panel {
                Polygon::rectangle(self.container_dims.width, self.container_dims.height)
                    .translate(top_left.x, top_left.y),
            );
+            g.unfork();


This is not related to my change. And didn't seem to be causing problems as is... but it seemed wrong where it was. I assumed all unfork should have a corresponding fork.

I'm only like 1/2 sure I understand what forking is even about - I think it's about nesting coordinate systems, akin to UIKit's#bounds.

I assumed all unfork should have a corresponding fork.

Correct, they should. Here must've been missing by mistake. unfork isn't strictly needed if there's a later explicit fork, which must've wound up being true here.

I think it's about nesting coordinate systems

fork is about making the GPU do coordinate transformations. At first, it was just "screen-space" and "map-space". We want to do map_to_screen inside the vertex shader for things that can pan and zoom, but not for screen-space UI things. Later I started using it also to shift/compress map-space, like for the minimap. It's possible some of those uses could instead do the work on the CPU, calling translate and scale on the GeomBatch. Haven't tried that. I'd assume it'd be much slower initially for things like the minimap, which has a large amount of geometry.

michaelkirk · 2020-10-01T23:57:24Z

widgetry/src/widgets/panel.rs

@@ -146,6 +227,7 @@ impl Panel {
        }

        if ctx.input.is_window_resized() {
+            self.update_container_dims_for_canvas_dims(ctx.canvas.get_window_dims());


this is another key change - don't miss it. 👀

dabreegster

This is a nice fix, thanks! I found some weird behavior if I launch something like the jump-to-time dialog and then resize:

But the behavior before this PR is also broken, just in a different way. Is this expected, or is it something you'd expect to be fixed?

Upon launch the window goes through a series of rapid size changes, starting small and growing to full size (this might just be a macos thing, I'm not sure?).

I think Linux does the same, maybe it's just Windows that initially gives a window with the full requested size. I don't remember exactly, but one of the bugs people reported right after alpha launch demonstrated the different window sizing behavior on diff platforms.

dabreegster · 2020-10-02T02:06:05Z

widgetry/src/widgets/panel.rs

+        // Wrap the main widget in scrollable containers if necessary.
+        if self.scrollable_x && !old_scrollable_x {
+            let mut place_holder = Widget::nothing();
+            std::mem::swap(&mut place_holder, &mut self.top_level);


You can do let placeholder = std::mem::replace(&mut self.top_level, Widget::nothing()). That's a slightly shorter form, equivalent to what you have here

dabreegster · 2020-10-02T02:10:53Z

widgetry/src/widgets/panel.rs

@@ -180,10 +262,9 @@ impl Panel {
                Polygon::rectangle(self.container_dims.width, self.container_dims.height)
                    .translate(top_left.x, top_left.y),
            );
+            g.unfork();


I assumed all unfork should have a corresponding fork.

Correct, they should. Here must've been missing by mistake. unfork isn't strictly needed if there's a later explicit fork, which must've wound up being true here.

I think it's about nesting coordinate systems

fork is about making the GPU do coordinate transformations. At first, it was just "screen-space" and "map-space". We want to do map_to_screen inside the vertex shader for things that can pan and zoom, but not for screen-space UI things. Later I started using it also to shift/compress map-space, like for the minimap. It's possible some of those uses could instead do the work on the CPU, calling translate and scale on the GeomBatch. Haven't tried that. I'd assume it'd be much slower initially for things like the minimap, which has a large amount of geometry.

michaelkirk · 2020-10-02T02:25:46Z

I found some weird behavior if I launch something like the jump-to-time dialog and then resize

Hmmm, nope that's not expected. I'll see if I can figure out what's going on.

michaelkirk · 2020-10-03T00:21:08Z

But the behavior before this PR is also broken, just in a different way. Is this expected, or is it something you'd expect to be fixed?

So the issue is that, similar to what this PR fixes for Panels, scrollbar (Slider) size and position is hardcoded at creation time.

As an optimization, I only created the scrollbars when they first become necessary, so their size and position was staying at whatever it was when the scrollbar is first drawn.

I'm now re-creating the sliders whenever a Panel re-computes it's layout, so it can now incorporate it's new size and location.

I spent maybe an hour fixing this problem. Then, wanting to make sure the perf wasn't terrible, spent another 3 hours digging into cargo flamegraph bugs on macos. 😞

The perf is pretty inconsequential even when regenerating the scrollbars all the time. If we did want to improve perf, I left a TODO about being smarter about when we actually need to recompute layout.

dabreegster

I spent maybe an hour fixing this problem. Then, wanting to make sure the perf wasn't terrible, spent another 3 hours digging into cargo flamegraph bugs on macos. disappointed

Oh no, yak shaving :(

If we did want to improve perf

My attitude so far has been, don't optimize it until it's noticeable. There's tons of low-hanging fruit to be more incremental in widgetry. No need yet, thankfully.

So your change makes sense, but I think it breaks things:

The scrollbars in the timeseries demo go away by default (they worked before this PR)
upon resizing the window, now the scrollbars show up in the right spot (fixed with this PR), but now all the scrollbars can't be moved (a regression)

I'll take a look at some of these this weekend

dabreegster · 2020-10-03T01:57:46Z

widgetry/examples/demo.rs

-                    let (bg_texture, fg_texture) = self.controls.dropdown_value("texture dropdown");
+                "apply" => {
+                    let (v_align, h_align) = self.controls.dropdown_value("alignment");
+                    self.controls.align(v_align, h_align);


Nice addition to the demo! I was a bit confused at first, forgot to hit "Apply". Dunno how much UX design we need to do for the demo -- I like demonstrating dropdown_value here.

TBH I don't know of problem currently being caused by this, but it seemed wrong to unfork without a matching fork.

"c" was holdover from when panels were called composites

michaelkirk · 2020-10-06T00:28:10Z

The scrollbars in the timeseries demo go away by default (they worked before this PR)

Ah, previously we were laying out twice when building the panel. I thought we could avoid it, but I was wrong. I've added it back and some comments explaining why here: 07117ab

upon resizing the window, now the scrollbars show up in the right spot (fixed with this PR), but now all the scrollbars can't be moved (a regression)

Shoot. I broke this while fixing this:

I found some weird behavior if I launch something like the jump-to-time dialog and then resize:

This fixes the inability to scroll by restoring old scroll offset to the new scrollbars: ffc5728

I'd like to do more to make panels responsive in the future - in particular, I think the big missing piece is allowing them to resize their content size without going through the "rebuild the panel and restore dance".

michaelkirk · 2020-10-06T00:28:23Z

This is ready for re-review PTAL @dabreegster

dabreegster · 2020-10-06T00:46:13Z

All my resizing tests work as expected; it's quite intuitive how a scrollbar grows/shrinks on a partly-scrolled panel. Nice!

I'd like to do more to make panels responsive in the future - in particular, I think the big missing piece is allowing them to resize their content size without going through the "rebuild the panel and restore dance".

SGTM. The other two related feature requests from Yuwen to keep in mind, that may relate to this implementation:

directives for Widgets to grow and fill remaining space. I don't exactly recall the use case, but there was probably a button whose width should be based on the parent container, not the contents of the button.
scrollbars on inner pieces of a panel, not necessarily the whole thing
You can probably guess why I haven't attempted either one yet. ^^;

michaelkirk · 2020-10-06T00:56:17Z

directives for Widgets to grow and fill remaining space. I don't exactly recall the use case, but there was probably a button whose width should be based on the parent container, not the contents of the button.

scrollbars on inner pieces of a panel, not necessarily the whole thing
You can probably guess why I haven't attempted either one yet. ^^;

exciting! these seem aligned with a longer term goal of mine to start leveraging relative position/size rather than only absolute sizes. I've only barely scratched the surface on the flex crate, but am hoping to get more familiar with it eventually.

michaelkirk commented Oct 1, 2020

View reviewed changes

dabreegster reviewed Oct 2, 2020

View reviewed changes

dabreegster reviewed Oct 3, 2020

View reviewed changes

michaelkirk force-pushed the mkirk/resize-top-center branch from 8b30015 to ab8ed68 Compare October 5, 2020 21:25

michaelkirk added 10 commits October 5, 2020 17:00

match unfork to fork

b53acf9

TBH I don't know of problem currently being caused by this, but it seemed wrong to unfork without a matching fork.

variable rename (no behavior change)

8a9e33e

"c" was holdover from when panels were called composites

resize container dims when window resizes

bab56f3

make panel.top_level private

ace3c5c

Show/Hide scrollbars as window is resized

6b07a5f

alignment demo

0a7648d

Fix scrollbar positioning

480db9f

separate out scrollbar work so we can profile it easier

718f693

mem::replace is more concise than mem::swap

671829c

fixup size of MaxPercent

07117ab

michaelkirk force-pushed the mkirk/resize-top-center branch from ab8ed68 to 90de843 Compare October 6, 2020 00:03

restore scroll position after redrawing scroll bars

ffc5728

michaelkirk force-pushed the mkirk/resize-top-center branch from 90de843 to ffc5728 Compare October 6, 2020 00:03

dabreegster merged commit 14cbb0f into a-b-street:master Oct 6, 2020

dabreegster deleted the mkirk/resize-top-center branch October 6, 2020 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update panels and their scrollbars when window resizes #358

update panels and their scrollbars when window resizes #358

michaelkirk commented Oct 1, 2020

michaelkirk Oct 1, 2020

michaelkirk Oct 1, 2020

dabreegster Oct 2, 2020

michaelkirk Oct 3, 2020

michaelkirk Oct 1, 2020 •

edited

Loading

dabreegster Oct 2, 2020

michaelkirk Oct 1, 2020

dabreegster left a comment

dabreegster Oct 2, 2020

dabreegster Oct 2, 2020

michaelkirk commented Oct 2, 2020

michaelkirk commented Oct 3, 2020 •

edited

Loading

dabreegster left a comment

dabreegster Oct 3, 2020

michaelkirk commented Oct 6, 2020 •

edited

Loading

michaelkirk commented Oct 6, 2020

dabreegster commented Oct 6, 2020

michaelkirk commented Oct 6, 2020

update panels and their scrollbars when window resizes #358

update panels and their scrollbars when window resizes #358

Conversation

michaelkirk commented Oct 1, 2020

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

michaelkirk Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Oct 2, 2020

michaelkirk commented Oct 3, 2020 • edited Loading

dabreegster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Oct 6, 2020 • edited Loading

michaelkirk commented Oct 6, 2020

dabreegster commented Oct 6, 2020

michaelkirk commented Oct 6, 2020

michaelkirk Oct 1, 2020 •

edited

Loading

michaelkirk commented Oct 3, 2020 •

edited

Loading

michaelkirk commented Oct 6, 2020 •

edited

Loading