Skip to content
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

Adds support for invalidating a single rectangle. #817

Merged
merged 7 commits into from
Apr 23, 2020
Merged

Adds support for invalidating a single rectangle. #817

merged 7 commits into from
Apr 23, 2020

Conversation

jneem
Copy link
Collaborator

@jneem jneem commented Apr 8, 2020

This adds support in druid-shell for invalidating a single rectangle.

Caveats/questions:

  • I checked that the win/mac changes compile, but I can't test them. Mac is particularly likely to be wrong, since I have no idea how rust-objc works, and how it maps to the apple docs online.
  • Should the plain invalidate methods be deprecated? Or should I just keep the name and introduce the rect parameter?

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks like a good direction, a couple comments inline.

My biggest concern is around scrolling; the logic for propagating invalidations is different for that than for most other containment.

Apologies my bandwidth for reviewing this in more detail is more limited than usual.

druid/src/contexts.rs Outdated Show resolved Hide resolved
druid/src/widget/invalidation.rs Show resolved Hide resolved
@jneem
Copy link
Collaborator Author

jneem commented Apr 13, 2020

So I had a first stab at getting scrolling working. I'm not super happy with it (which is why I only implemented EventCtx so far), but it's the best I have right now.

As far as I understand, the way scrolling eventually works is the following: the scroll widget will maintain a buffer that contains the painted contents of its children. That buffer will be somewhat bigger than the viewport. When the Scroll is scrolled, it just blits from the buffer. It can handle asking its child to rerender the buffer on its own time, possibly even as an idle callback.

This means that:

  • if a child of the scroll requests repainting some visible rect, the scroll needs to propagate that back up to the system, properly translated; but also,
  • the scroll needs to store the invalidated regions somewhere, in order do do its own repainting.

AFAICT, these two constraints (and especially the second) means that the Scroll really needs to insert a callback somewhere. And this was the least-ugly place I could find to put it.

@jneem
Copy link
Collaborator Author

jneem commented Apr 13, 2020

By the way, the current implementation has a buglet, in that there is a situation where it invalidates too much (and I don't know how to fix it without more drastic changes): if the scroll has a Flex child, and that child invalidates something in the viewport and something out of the viewport, then the bounding box of those two invalidations will be quite large (but this isn't the bug yet). The bug is that the part outside the viewport won't get painted, because Flex doesn't paint its invisible children. That means it will never get uninvalidated (validated?), and so that large bounding box will keep getting repainted.

I'm not planning to do anything about this right away, I just wanted to right it down so I don't forget...

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some notes and questions, but I think this is in good shape should should be ready to go soon!

druid-shell/src/platform/mac/window.rs Outdated Show resolved Hide resolved
druid-shell/src/window.rs Outdated Show resolved Hide resolved
druid/examples/scroll.rs Outdated Show resolved Hide resolved
druid/src/contexts.rs Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,67 @@
// Copyright 2020 The xi-editor Authors.
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way I might consider implementing this is not as a separate widget, but as part of WidgetPod's paint method? It's possible this doesn't make sense, but it's my first instinct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally tried to imitate the implementation of debug_paint_layout (i.e. using EnvScope and WidgetPod::paint instead of a new widget). The problem is that, unlike drawing layout-rect boundaries, we don't want to recursively draw the invalidation regions for the children. It just occurred to me, though, that I can probably use Painter instead of a new widget.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually thinking about this more (and further into my review), maybe it does make sense to draw them recursively? This would clarify where the invalidation calls that produced the final result came from?

In this world, I might also just draw outlines of the invalidated regions, so that we don't end up stacking up the alpha channel and obscuring the view.

druid/src/window.rs Outdated Show resolved Hide resolved
druid/src/window.rs Outdated Show resolved Hide resolved
druid/src/widget/invalidation.rs Outdated Show resolved Hide resolved
@cmyr cmyr added the S-waiting-on-author waits for changes from the submitter label Apr 17, 2020
@jneem
Copy link
Collaborator Author

jneem commented Apr 17, 2020

Thanks for the review! I'll do a round or two of clean-up and then remove the [WIP]. I'd still feel better if someone briefly tested windows/mac, though...

1 similar comment
@jneem
Copy link
Collaborator Author

jneem commented Apr 17, 2020

Thanks for the review! I'll do a round or two of clean-up and then remove the [WIP]. I'd still feel better if someone briefly tested windows/mac, though...

@jneem jneem changed the title [WIP] Adds support for invalidating a single rectangle. Adds support for invalidating a single rectangle. Apr 19, 2020
@jneem
Copy link
Collaborator Author

jneem commented Apr 20, 2020

The remaining CI failures appear to be linking errors that I can't reproduce locally...

Fixes #402.

Widgets can request partial invalidation using `request_paint_rect`.
All of the partial invalidation requests will be combined into a
single bounding rectangle, and passed to the system. When painting
widgets, they have access to the dirty region.

With this commit, widgets that paint outside their paint rects can
cause graphical glitches. These widgets are buggy, but their bugs
weren't so visible before (see #628).
@jneem
Copy link
Collaborator Author

jneem commented Apr 21, 2020

Ok, seems like a rebase resolved it.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay the code looks generally good but playing with the example I'm not sure I'm seeing the expected behaviour (on mac).

druid-shell/examples/invalidate.rs Outdated Show resolved Hide resolved
druid-shell/examples/invalidate.rs Outdated Show resolved Hide resolved
let color = env.get_debug_color(self.debug_color).with_alpha(0.5);
let rect = ctx.region().to_rect();
ctx.fill(rect, &color);
self.debug_color += 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the rationale for changing the color, here? to me it makes sense to have the color be unchanged between paint cycles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the color change, every time a region gets invalidated it gets painted with a blue (say) overlay. The overall effect is that the entire window is covered with a blue overlay all the time, so you can't see what was just invalidated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused; wouldn't that only be the case if the entire view was invalidated?

druid/src/widget/mod.rs Outdated Show resolved Hide resolved
ctx.fill(Circle::new(*data, RADIUS), &Color::BLACK);
})
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so on macOS, this example is not displaying any visible minimal invalidation; on click the whole screen is tinted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's not right. Does the invalidation test in druid_shell work? It should look something like the attached gif.
invalidation_test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, on mac it is just a window with a single solid background color. I'll dig in a little bit...

@@ -0,0 +1,67 @@
// Copyright 2020 The xi-editor Authors.
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually thinking about this more (and further into my review), maybe it does make sense to draw them recursively? This would clarify where the invalidation calls that produced the final result came from?

In this world, I might also just draw outlines of the invalidated regions, so that we don't end up stacking up the alpha channel and obscuring the view.

@jneem
Copy link
Collaborator Author

jneem commented Apr 21, 2020

I'll try out a few options for the debug visualization. I don't think recursive drawing will work well, because there's just one single invalidation rect for the entire window, and so any individual widget's invalidation rect is just its paint rect intersected with the global rect -- there's no interesting recursive structure like there is for the layout rects.

I'll also try doing outlines.

@jneem
Copy link
Collaborator Author

jneem commented Apr 21, 2020

By the way, this is what the (druid) invalidation test looks like for me.
druid_invalidation

@jneem
Copy link
Collaborator Author

jneem commented Apr 21, 2020

Here are two other options: recursive outlines and non-recursive outlines. I think my favorite is the non-recursive outlines.
single_outline
recursive_outline

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I found two small issues keeping the examples from working on mac.

With these changes, this works as intended.

One other thing I noticed digging in a bit more deeply is that animation always invalidates everything; I think this is okay for now but we should open an issue about it when this gets merged.

druid-shell/src/platform/mac/window.rs Outdated Show resolved Hide resolved
Comment on lines 50 to 60
fn connect(&mut self, handle: &WindowHandle) {
self.handle = handle.clone();
}

fn paint(&mut self, piet: &mut Piet, rect: Rect) -> bool {
self.update_color_and_rect();
piet.fill(rect, &self.color);

self.handle.invalidate_rect(self.rect);
false
}
Copy link
Member

@cmyr cmyr Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on mac, invalidation calls that happen during drawing are ignored.

I would suggest changing this to use a timer, something like:

    fn connect(&mut self, handle: &WindowHandle) {
        self.handle = handle.clone();
        self.handle.request_timer(std::time::Duration::from_millis(60));
    }
    
    fn timer(&mut self, _id: TimerToken) {
        self.update_color_and_rect();
        self.handle.invalidate_rect(self.rect);
        self.handle.request_timer(std::time::Duration::from_millis(60));
    }

    fn paint(&mut self, piet: &mut Piet, rect: Rect) -> bool {
        piet.fill(rect, &self.color);
        false
    }

@jneem
Copy link
Collaborator Author

jneem commented Apr 22, 2020

Thanks! I fixed those, and also some other issues with propagating a too-large invalid region in druid (in cairo, the drawing context is automatically clipped, so I wasn't seeing the issue).

Regarding the animation, there was a reason I hadn't done it yet, but I can't remember why...

@cmyr
Copy link
Member

cmyr commented Apr 22, 2020

I wouldn't worry about the animation, it will be sort of tricky to get right with the current API. The longer-term plan is to build a higher-level API on top of what we currently have, and when we do that we can revisit.

I guess it would be possible to make some specific improvements now, though; but this would require turning request_anim into a Region or something.

@cmyr
Copy link
Member

cmyr commented Apr 22, 2020

Also I'm not sure I can tell the difference between the recursive v. non-recursive outlines; I defer to your judgement.

@jneem
Copy link
Collaborator Author

jneem commented Apr 22, 2020

Ok, so I've gone with non-recursive outlines then. It's easy enough to change later anyway.

Also, I'll open an issue about request_anim.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for your patience on this, I'm satisfied. I'll ping @raphlinus and see if he wants to take a look, I think we're looking good though.

Just out of curiousity, have you used this and found a measurable improvement in performance?

@cmyr
Copy link
Member

cmyr commented Apr 22, 2020

Oh, one last thing: it might be worth adding this to the wasm examples? in druid/examples/wasm. (forgive me if this is don't, I skimmed and didn't see i)

@jneem
Copy link
Collaborator Author

jneem commented Apr 22, 2020

I think it's automatic, right? druid/examples/wasm/src/examples seems to be symlinked to druid/examples.

Edit: although now that I'm testing the wasm example, the whole thing seems to get invalidated...

@jneem
Copy link
Collaborator Author

jneem commented Apr 22, 2020

Ok, I think I may have fixed the wasm example. I am a bit puzzled by the two calls to piet_ctx.finish() in druid_shell::platform::web::WindowState::render() -- is that just an oversight?

I had a toy example showing improved performance for small invalidations when drawing is expensive. I can try cleaning it up and adding it as an example. But my real motivation for doing this needs more work before it can benefit...

@cmyr
Copy link
Member

cmyr commented Apr 23, 2020

I can't speak to the wasm stuff; maybe @elrnv can. In any case I'm happy to merge this now, thanks again for your patience!

@cmyr cmyr merged commit fec0f0e into linebender:master Apr 23, 2020
@elrnv
Copy link
Contributor

elrnv commented Apr 23, 2020

I am not sure why we need to finish calls. For reference, I believe the code @jneem is referring to is in the render function

fn render(&self) -> bool {
    self.context
        .clear_rect(0.0, 0.0, self.get_width() as f64, self.get_height() as f64);
    let mut piet_ctx = piet_common::Piet::new(self.context.clone(), self.window.clone());
    let want_anim_frame = self.handler.borrow_mut().paint(&mut piet_ctx);
    if let Err(e) = piet_ctx.finish() {
        log::error!("piet error on render: {:?}", e);
    }
    let res = piet_ctx.finish();
    if let Err(e) = res {
        log::error!("EndDraw error: {:?}", e);
    }
    want_anim_frame
}

This part is over a year old. It does look redundant to me. After trying the examples without the second piet_ctx.finish(), I couldn't find any difference. I think it's safe to remove the second .finish() call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants