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

Add the code for vello_cpu #830

Merged
merged 21 commits into from
Mar 5, 2025
Merged

Add the code for vello_cpu #830

merged 21 commits into from
Mar 5, 2025

Conversation

LaurenzV
Copy link
Contributor

@LaurenzV LaurenzV commented Mar 4, 2025

Doesn't include the tests yet which will come after this PR. But I verified that the tests pass using this setup.

@DJMcNab
Copy link
Member

DJMcNab commented Mar 4, 2025

(the tests are excluded at my request, because we need to make sure that the LFS settings are all done properly)

Copy link
Member

@tomcur tomcur left a comment

Choose a reason for hiding this comment

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

This looks good. I put some thoughts inline.

The RenderContext should probably change to having a transform stack (and in the future a clip stack as well), allowing somewhat more efficient pushing and popping of layers, but that's not a blocker for this PR. See e.g. this comment:

The initial API should not be trait based. Like Bintje, I have code in a private branch that stores the transform stack so that is part of the intent of the design. (the code I have is for a tiny-skia backend; I don't think I've published this anywhere. Taking a quick look at the code from Bintje, the logic is similar enough there's not a lot of value in mine). And yes, let's have a separate issue for the medium to longer term API considerations.

As a general note, my preference would be to move away from encoding things in the pipeline as u8 and u32, unless there's a strong reason for doing that. Alpha masks could be encoded as Vec<u8> (being generic over tile height almost for free), and e.g. pixmaps could be Vec<PremulRgba8>. When interfacing with a GPU we can bytemuck and also ensure endianness is correct at that interface boundary.

@LaurenzV
Copy link
Contributor Author

LaurenzV commented Mar 5, 2025

The RenderContext should probably change to having a transform stack (and in the future a clip stack as well), allowing somewhat more efficient pushing and popping of layers, but that's not a blocker for this PR.

Yeah, that makes sense!

As a general note, my preference would be to move away from encoding things in the pipeline as u8 and u32, unless there's a strong reason for doing that.

Yes, we definitely have to address that once we want to support f32-based rendering, though as mentioned before I would leave that for once the basic structure is set up.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

Generally looks good, and think in generally we should land things early and then iterate.

I agree with Tom that more precise types for things (colors, chunks of alpha values). Part of the reason why the prototypes use primitive types is bad habits from GPU land. But Tom's suggestion is valid: on the Rust side we should use good types, then depend on bytemuck to bit-reinterpret the types to u32 etc.

@LaurenzV LaurenzV added this pull request to the merge queue Mar 5, 2025
Merged via the queue into main with commit 1b6904e Mar 5, 2025
17 checks passed
@LaurenzV LaurenzV deleted the vello_cpu branch March 5, 2025 19:58
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.

4 participants