-
Notifications
You must be signed in to change notification settings - Fork 156
Add support for rendering images #919
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
Conversation
@DJMcNab Regarding the image assets for the tests I added, is it fine to store them where I did? Would you like to see them stored somewhere else instead? |
sparse_strips/vello_api/Cargo.toml
Outdated
@@ -13,6 +13,7 @@ publish = false | |||
|
|||
[dependencies] | |||
peniko = { workspace = true } | |||
png = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this? Could we move the responsibility of preparing the image data to the client? If it still provides value, perhaps we could consider making it an optional feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that loading png images is such a common operation, I think it would be good to keep this. But I will definitely make it an optional feature.
# Conflicts: # sparse_strips/vello_common/src/encode.rs # sparse_strips/vello_common/src/pixmap.rs # sparse_strips/vello_cpu/src/fine/mod.rs
fed3369
to
c3a3505
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stored in-repo images are fine - they're small, and not stored in LFS, which are the properties we need for test input.
repeat!("image_pad_x_pad_y", Extend::Pad, Extend::Pad); | ||
} | ||
|
||
macro_rules! transform { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I missed these macros — honestly, I find things a lot easier to read/debug when it’s just plain functions, unless using macros truly adds value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning on removing all the macros in #925, would it be okay if I defer it until then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely!
What... Two tests are only failing on Windows? 😅 |
I will just exclude them for now, no idea what's going on. |
Can you make an issue for the failing tests please? |
No description provided.