Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions vello/src/scene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,59 @@ impl Scene {
self.encoding.encode_end_clip();
}

/// Compute a rough bounding box of the control points in the scene.
/// This is not axis-aligned or precise.
Copy link
Member

Choose a reason for hiding this comment

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

How can it be non-axis aligned? The Rect type is inherently axis-aligned, as far as I can tell. Should it also return a rotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

My goal isn't to compute an AABB, just a BB since I just need to do basic viewport culling with the BB.

By axis aligned I mean the rect will not rotate to fit the paths it contains better.

Copy link
Member

Choose a reason for hiding this comment

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

What does precise mean here? Do you mean that it has false positives and negatives, or only that it isn't tight? The code reads to me that it intentionally has both false positives and false negatives, at which point I struggle to see the advantage of this code

Copy link
Member Author

Choose a reason for hiding this comment

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

Not precise means it isn't "tight"; it doesn't wrap the paths, it wraps the control points.

pub fn compute_bb(&self) -> Rect {
if self.encoding.path_data.is_empty() {
return Rect::ZERO;
}

let mut min_x = f32::INFINITY;
let mut min_y = f32::INFINITY;
let mut max_x = f32::NEG_INFINITY;
let mut max_y = f32::NEG_INFINITY;

// Treat consecutive u32 pairs as potential x,y coordinates
for chunk in self
.encoding
.path_data
.chunks_exact(2)
.chain(self.encoding.draw_data.chunks_exact(2))
{
let &[x, y] = chunk else {
continue;
};
let x = f32::from_bits(x);
let y = f32::from_bits(y);
Comment on lines +148 to +152
Copy link
Member

Choose a reason for hiding this comment

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

This isn't valid if there are i16 coordinates. See also #819.
As I said in that PR, removing the i16 coordinates for now would be acceptable (but should probably be its own PR), or we need to document that we don't handle them properly


// Only consider finite values as valid coordinates
if x.is_finite() && y.is_finite() {
min_x = min_x.min(x);
min_y = min_y.min(y);
max_x = max_x.max(x);
max_y = max_y.max(y);
}
Copy link
Member

Choose a reason for hiding this comment

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

Again, see also #819 - maybe we could return None or an error value if there are non-finite values in the path?

}

// Check glyph positions if any
for glyph in &self.encoding.resources.glyphs {
let x = glyph.x;
let y = glyph.y;
min_x = min_x.min(x);
min_y = min_y.min(y);
max_x = max_x.max(x);
max_y = max_y.max(y);
}
Comment on lines +164 to +171
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite follow how this code gets the bounds of the glyphs. If I have a glyph, it won't just be a 0x0 pixel box?

Copy link
Member Author

@simbleau simbleau May 27, 2025

Choose a reason for hiding this comment

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

Yeah, I agree this code looks wrong. I wasn't sure how to get the bounds of the glyphs cheaply without shaping/etc. so I just took the xy as a control point.

Once again this refers to the BB not being precise.


// If we found no valid coordinates, return zero.
if min_x.is_infinite() || min_y.is_infinite() || max_x.is_infinite() || max_y.is_infinite()
{
return Rect::ZERO;
}

Rect::new(min_x as f64, min_y as f64, max_x as f64, max_y as f64)
}

/// Draw a rounded rectangle blurred with a gaussian filter.
pub fn draw_blurred_rounded_rect(
&mut self,
Expand Down
66 changes: 66 additions & 0 deletions vello_tests/tests/scene.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright 2024 the Vello Authors
// SPDX-License-Identifier: Apache-2.0 OR MIT
//! Simple testing of vello scenes.
// The following lints are part of the Linebender standard set,
// but resolving them has been deferred for now.
// Feel free to send a PR that solves one or more of these.
#![allow(
clippy::missing_assert_message,
clippy::allow_attributes_without_reason
)]
Comment on lines +4 to +10
Copy link
Member

Choose a reason for hiding this comment

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

This is entirely new code, and so should follow the usual patterns

use vello::Scene;
use vello::kurbo::{Affine, Ellipse, Rect};
use vello::peniko::{Brush, color::palette};

#[test]
fn simple_square() {
let mut scene = Scene::new();
scene.fill(
vello::peniko::Fill::NonZero,
Affine::IDENTITY,
&Brush::Solid(palette::css::RED),
None,
&Rect::from_center_size((-25., 0.), (50., 50.)),
);
let actual = scene.compute_bb();
let expected = Rect::new(-50.0, -25.0, 0.0, 25.0);
assert_eq!(expected, actual);
}

#[test]
fn simple_ellipse() {
let mut scene = Scene::new();
scene.fill(
vello::peniko::Fill::NonZero,
Affine::IDENTITY,
&Brush::Solid(palette::css::RED),
None,
&Ellipse::new((0.0, 0.0), (50.0, 20.0), 0.0),
);
let actual = scene.compute_bb();
let expected = Rect::new(-50.0, 50.0, -20.0, 20.0);
assert_eq!(expected, actual);
}

#[test]
fn rotated_ellipse() {
let mut scene = Scene::new();
scene.fill(
vello::peniko::Fill::NonZero,
Affine::IDENTITY,
&Brush::Solid(palette::css::RED),
None,
&Ellipse::new((0.0, 0.0), (50.0, 20.0), 90.0),
);
let actual = scene.compute_bb();
let expected = Rect::new(-20.0, -50.0, 20.0, 50.0);
assert_eq!(expected, actual);
}

#[test]
fn empty_scene() {
let scene = Scene::new();
let actual = scene.compute_bb();
let expected = Rect::ZERO;
assert_eq!(expected, actual);
}
Loading