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

[WIP] Add damage tracking, fixes #367 #377

Closed
wants to merge 4 commits into from

Conversation

valpackett
Copy link
Contributor

@valpackett valpackett commented Jun 1, 2020

fixes #367

Experimenting with implementing damage by "diffing" Primitives.

  • primitives' bounds is not what I expected, e.g. the x of a centered text box under the slider in the tour example has its x in the middle of the screen o_0
  • be more precise by not joining everything into one rect
    • but for debugging one rect is easier..
  • don't use unstable fold_first (iterator_fold_self)
  • wgpu for now should at least skip rendering when there's no damage
  • why do mouse clicks always cause whole screen damage?
  • with the compositor hints (swap_buffers_with_damage) we lose the winit border hover effects (we don't damage for them).. fix this somehow?
  • mouse interaction when skipping rendering due to zero damage ?? why is mouse cursor determined by actual pixel rendering?

Demo: Wayfire -d damage debug view, couple examples, joined rect; now with many rects!

@valpackett valpackett force-pushed the damage branch 2 times, most recently from 0aff397 to dbf1890 Compare June 1, 2020 18:10
@Imberflur
Copy link
Contributor

@myfreeweb I think the text bounds are an artifact of preparing the bounds for the behavior of glyph_brush alignment. I ran into this when implementing a renderer for iced. It is pretty wacky imo...

@valpackett
Copy link
Contributor Author

@Imberflur yeah. This should at least be documented in the Primitive::Text doc string. Or, indeed, pushed out closer to interacting with that crate, since we have the alignments stored in the Primitive::Text anyway.

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Albeit a bit early, it's great to see some experimentation with this! 🎉

I really like the idea of a first-class Primitive because it allows us to reason about rendering data at runtime and implement strategies like this one!

However, as I said in #241, I am not sure we should be focusing on optimization yet. There are many features still missing and it's unclear how they will end up affecting the overall architecture. For instance, we may end up removing the Primitive type altogether and instead use a mutable renderer context approach, which could help us avoid sparse allocations (or maybe we could use something like bumpalo).

In any case, the good thing is that this approach is relatively "runtime-agnostic", so it barely affects the codebase. I believe it could be very promising once we combine it with primitive culling at the Renderer level.

The main issue here is that we do not have any meaningful benchmarks to understand the tradeoffs. It'd be great to have some numbers!

why is mouse cursor determined by actual pixel rendering?

The mouse interaction is a detail of the renderer implementation. This allows us to create renderers where mouse interactions are not possible (like text-based ones). The work for this is currently half-way, though.

Why is it unwrapped during draw? No particular reason. As we are redrawing all the time, it was easy to make draw unwrap it and return it. We should definitely change it as soon as we choose to optimize.

I think the text bounds are an artifact of preparing the bounds for the behavior of glyph_brush alignment. I ran into this when implementing a renderer for iced. It is pretty wacky imo...

I intentionally implemented this behavior here: #281

It allows us to represent unbounded text with alignment without measuring it first. We should probably change the Rectangle type to make this distinction explicit. If you have any other suggestions, I'd love to hear them.

This should at least be documented in the Primitive::Text doc string

I'll add it once I get a chance.

@hecrj hecrj added the improvement An internal improvement label Jun 1, 2020
@Imberflur
Copy link
Contributor

I intentionally implemented this behavior here: #281

It allows us to represent unbounded text with alignment without measuring it first. We should probably change the Rectangle type to make this distinction explicit. If you have any other suggestions, I'd love to hear them.

Thanks for the clarification! That makes a lot of sense. My main annoyance with this was how a seemingly unrelated parameter in glyph_brush affected the meaning of the position. I think clearer docs for Section could help with that. Not really related to iced though.

@valpackett
Copy link
Contributor Author

huh, looks like the winit header bar fixed itself after rebase

}
if self != other {
match (self.bounds(), other.bounds()) {
(Some(lb), Some(rb)) if lb != rb => Some(vec![lb, rb]),

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if the primitives are actually different types? E.g. text vs quad, but have the same boundaries?

We only care about the boundaries, why would it matter what primitive they're from?

in a Group, the first primitive is suddenly removed which shifts all primitives so that they invalidate the assumption that they line up perfectly (referring to the .zip() on line 164).

If a primitive is only removed, the lengths won't match, so we're no longer in lprims.len() == rprims.len().

If it's replaced with a differently sized one, nothing would line up and the lp.damage(rp) calls would damage everything.

Copy link

Choose a reason for hiding this comment

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

Hm I see, makes sense.

@hecrj
Copy link
Member

hecrj commented Feb 12, 2021

As we are not ready to write code for this yet, I believe we can close this for now.

This should probably land some time after #553. We can start additional discussions over Zulip or in an issue.

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

Successfully merging this pull request may close these issues.

Damage tracking / partial present
4 participants