Skip to content

sparse_strips: Move some code from vello_cpu to vello_common#1744

Open
LaurenzV wants to merge 9 commits into
mainfrom
laurenz/vello_hybrid_shift_part1
Open

sparse_strips: Move some code from vello_cpu to vello_common#1744
LaurenzV wants to merge 9 commits into
mainfrom
laurenz/vello_hybrid_shift_part1

Conversation

@LaurenzV

@LaurenzV LaurenzV commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

This is the first PR in a series of three (perhaps four) PRs that serve as a preparation for the Vello Hybrid rewrite.

This PR is easiest reviewed commit by commit, though there isn't that much to review in the first place. Apart from adjusting a few descriptions, it's mostly just moving some code around.

The first 7 commits were done by manually copy-pasting code into the right location. The last commit that fixes compiler issues were done with AI assistance.

PR train

  1. sparse_strips: Move some code from vello_cpu to vello_common #1744 — Move some code from vello_cpu to vello_common 👈
  2. sparse_strips: Add more new/refactored code into vello_common #1745 — Add more new/refactored code into vello_common
  3. vello_common: Generalize the command recorder abstraction and add more metadata #1746 — Generalize the command recorder abstraction and add more metadata
  4. sparse_strips: Introduce Padding type and streamline clip handling #1751 — Introduce Padding and streamline clip handling

Comment on lines +178 to +187
// This struct implements an additional piece of logic to make non-isolated clips work properly
// with filter layers. The root of all "evil" that requires us to implement this wrapper around
// [`crate::clip::ClipContext`] is that, as the user pushes new filter layers into the
// render context, we eagerly apply a shift to all subsequently rendered contents to ensure that
// everything necessary for correct filter rendering is guaranteed to be visible. However, since
// the clip stack eagerly generates strips for each clip path that is clipped to the original
// viewport, those generated clip paths cannot just be translated on demand to account for the
// source shift of the filter layer. Therefore, every time a new filter layer is pushed, we need
// to regenerate the clip context for that specific layer to ensure clips are applied correctly.
/// State for managing clip paths across multiple viewports.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was slightly rewritten from the original doc comment.

Comment on lines +6 to +7
//! Note: The description below was written with Vello CPU in mind, but also applies to Vello
//! Hybrid, except that Vello Hybrid does not perform bucketing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this disclaimer worth it? Could we instead just update the comments wherever the behavior differs? Otherwise, it adds extra mental overhead and distracts from the purpose of vello_common as the shared place for code used by both renderers.

@grebmeg

grebmeg commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Btw, it might be worth keeping this branch (or maybe main-vello-hybrid-filter-fix) as a consolidated branch so it's possible to review the entire change one more time in one place. PR train makes reviewing large changes much easier, but it also makes it harder to see how the whole implementation fits together. If you spot an issue later and the relevant PR has already been merged, it becomes more difficult to fix it or, in the worst case, it'd need a hotfix.

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.

2 participants