clamp the damage rect to the destination rect #449
Conversation
src/backend/renderer/gles2/mod.rs
Outdated
| .map(|rect| { | ||
| let rect = rect.to_f64(); | ||
|
|
||
| let rect_constrained_loc = rect | ||
| .loc | ||
| .constrain(Rectangle::from_extemities((0f64, 0f64), dest.size.to_point())); | ||
| let rect_clamped_size = rect.size.clamp((0f64, 0f64), dest.size); | ||
|
|
||
| [ | ||
| rect.loc.x as f32 / dest.size.w as f32, | ||
| rect.loc.y as f32 / dest.size.h as f32, | ||
| rect.size.w as f32 / dest.size.w as f32, | ||
| rect.size.h as f32 / dest.size.h as f32, | ||
| (rect_constrained_loc.x / dest.size.w) as f32, | ||
| (rect_constrained_loc.y / dest.size.h) as f32, | ||
| (rect_clamped_size.w / dest.size.w) as f32, | ||
| (rect_clamped_size.h / dest.size.h) as f32, | ||
| ] | ||
| }) |
There was a problem hiding this comment.
How is that different from this?
let viewport = Rectangle::from_extemities((0f64, 0f64), dest.size.to_point());
let damage = damage
.iter()
.flat_map(|rect| rect.to_f64().intersection(viewport).map(|rect_constrained| {
[
(rect_constrained.loc.x / dest.size.w) as f32,
(rect_constrained.loc.y / dest.size.h) as f32,
(rect_constrained.size.w / dest.size.w) as f32,
(rect_constrained.size.h / dest.size.h) as f32,
]
}))
.flatten()
.collect::<Vec<_>>();There was a problem hiding this comment.
I changed it to use intersection and added a debug_assert! that the intersection is not None.
This could prevent some bugs where the submitted damage is completely outside of the surface.
cmeissl
force-pushed
the
fix/desktop_abstraction_damage_clamping
branch
from
9f02bea to
f1dce26
Compare
this fixes issues when the damage rect is greater than the destination rect, like providing i32::Max as the damage size
cmeissl
force-pushed
the
fix/desktop_abstraction_damage_clamping
branch
from
f1dce26 to
e9904ce
Compare
| (rect_constrained.size.h / dest.size.h) as f32, | ||
| ] | ||
| }); | ||
| debug_assert!(damage.is_some()); |
There was a problem hiding this comment.
Why this debug assert? Do we make it a fatal error to provide damage that is outside the viewport?
There was a problem hiding this comment.
The damage is what is used for rendering in render_texture defining the instances for instanced rendering. If the damage is outside of the damage_viewport in the worst case nothing will be drawn. Imo this is an error in the calling code but may be hard to find which this debug_assert! aims to make easier.
There was a problem hiding this comment.
what's your opinion on that @Drakulix ?
There was a problem hiding this comment.
Our code should never do this and it would be nice to catch cases early.
I would say we leave it in, until someone opens an issue with a legit use-case for providing non-overlapping damage values. Because I can't think of any.
this fixes issues when the damage rect is greater
than the destination rect, like providing i32::Max as
the damage size
@Drakulix @vberger
I have added two functions in
geometry.rs, if they are not entirely clear or do not fit i can remove them.