Skip to content

Commit

Permalink
PaintCallbackInfo::viewport_in_pixels now guarantees to stay in bounds (
Browse files Browse the repository at this point in the history
#3604)

* Fixes rerun-io/rerun#4297
* tested against a very hasty and incomplete port of egui/trunk, found
[here](https://github.com/rerun-io/rerun/tree/andreas/experimental-egui-trunk)


In rare cases in can happen that the viewport returned by
`PaintCallbackInfo` is outside the bounds of the screen.
for at least [wgpu/webgpu in
particular](https://www.w3.org/TR/webgpu/#dom-gpurenderpassencoder-setviewport)
this is invalid usage, other backends might be affected as well.
Since this happened due to a float rounding error (in one repro case I
had I got (width==1126.5625) + (offset=715.4376) = 1842.0001 for a
resolution of 1842) I decided to do away with fractional values on the
viewport alltogether. They _technically_ make sense since a viewport is
only specifying the NDC to pixel coordinate conversion, but practically
this may lead to surprising sub-sampling issues.

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
  • Loading branch information
Wumpf and emilk authored Nov 22, 2023
1 parent 6490dfa commit f9f5db9
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 23 deletions.
8 changes: 4 additions & 4 deletions crates/egui-wgpu/src/renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -475,10 +475,10 @@ impl Renderer {
let viewport_px = info.viewport_in_pixels();

render_pass.set_viewport(
viewport_px.left_px,
viewport_px.top_px,
viewport_px.width_px,
viewport_px.height_px,
viewport_px.left_px as f32,
viewport_px.top_px as f32,
viewport_px.width_px as f32,
viewport_px.height_px as f32,
0.0,
1.0,
);
Expand Down
8 changes: 4 additions & 4 deletions crates/egui_glow/src/painter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,10 +404,10 @@ impl Painter {
let viewport_px = info.viewport_in_pixels();
unsafe {
self.gl.viewport(
viewport_px.left_px.round() as _,
viewport_px.from_bottom_px.round() as _,
viewport_px.width_px.round() as _,
viewport_px.height_px.round() as _,
viewport_px.left_px,
viewport_px.from_bottom_px,
viewport_px.width_px,
viewport_px.height_px,
);
}

Expand Down
69 changes: 54 additions & 15 deletions crates/epaint/src/shape.rs
Original file line number Diff line number Diff line change
Expand Up @@ -803,44 +803,83 @@ pub struct PaintCallbackInfo {
pub screen_size_px: [u32; 2],
}

/// Size of the viewport in whole, physical pixels.
pub struct ViewportInPixels {
/// Physical pixel offset for left side of the viewport.
pub left_px: f32,
pub left_px: i32,

/// Physical pixel offset for top side of the viewport.
pub top_px: f32,
pub top_px: i32,

/// Physical pixel offset for bottom side of the viewport.
///
/// This is what `glViewport`, `glScissor` etc expects for the y axis.
pub from_bottom_px: f32,
pub from_bottom_px: i32,

/// Viewport width in physical pixels.
pub width_px: f32,
pub width_px: i32,

/// Viewport height in physical pixels.
pub height_px: f32,
pub height_px: i32,
}

impl PaintCallbackInfo {
fn pixels_from_points(&self, rect: &Rect) -> ViewportInPixels {
ViewportInPixels {
left_px: rect.min.x * self.pixels_per_point,
top_px: rect.min.y * self.pixels_per_point,
from_bottom_px: self.screen_size_px[1] as f32 - rect.max.y * self.pixels_per_point,
width_px: rect.width() * self.pixels_per_point,
height_px: rect.height() * self.pixels_per_point,
impl ViewportInPixels {
fn from_points(rect: &Rect, pixels_per_point: f32, screen_size_px: [u32; 2]) -> Self {
// Fractional pixel values for viewports are generally valid, but may cause sampling issues
// and rounding errors might cause us to get out of bounds.

// Round:
let left_px = (pixels_per_point * rect.min.x).round() as i32; // inclusive
let top_px = (pixels_per_point * rect.min.y).round() as i32; // inclusive
let right_px = (pixels_per_point * rect.max.x).round() as i32; // exclusive
let bottom_px = (pixels_per_point * rect.max.y).round() as i32; // exclusive

// Clamp to screen:
let screen_width = screen_size_px[0] as i32;
let screen_height = screen_size_px[1] as i32;
let left_px = left_px.clamp(0, screen_width);
let right_px = right_px.clamp(left_px, screen_width);
let top_px = top_px.clamp(0, screen_height);
let bottom_px = bottom_px.clamp(top_px, screen_height);

let width_px = right_px - left_px;
let height_px = bottom_px - top_px;

Self {
left_px,
top_px,
from_bottom_px: screen_height - height_px - top_px,
width_px,
height_px,
}
}
}

#[test]
fn test_viewport_rounding() {
for i in 0..=10_000 {
// Two adjacent viewports should never overlap:
let x = i as f32 / 97.0;
let left = Rect::from_min_max(pos2(0.0, 0.0), pos2(100.0, 100.0)).with_max_x(x);
let right = Rect::from_min_max(pos2(0.0, 0.0), pos2(100.0, 100.0)).with_min_x(x);

for pixels_per_point in [0.618, 1.0, std::f32::consts::PI] {
let left = ViewportInPixels::from_points(&left, pixels_per_point, [100, 100]);
let right = ViewportInPixels::from_points(&right, pixels_per_point, [100, 100]);
assert_eq!(left.left_px + left.width_px, right.left_px);
}
}
}

impl PaintCallbackInfo {
/// The viewport rectangle. This is what you would use in e.g. `glViewport`.
pub fn viewport_in_pixels(&self) -> ViewportInPixels {
self.pixels_from_points(&self.viewport)
ViewportInPixels::from_points(&self.viewport, self.pixels_per_point, self.screen_size_px)
}

/// The "scissor" or "clip" rectangle. This is what you would use in e.g. `glScissor`.
pub fn clip_rect_in_pixels(&self) -> ViewportInPixels {
self.pixels_from_points(&self.clip_rect)
ViewportInPixels::from_points(&self.clip_rect, self.pixels_per_point, self.screen_size_px)
}
}

Expand Down

0 comments on commit f9f5db9

Please sign in to comment.