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

Image x/y extend, alpha, nearest neighbor sampling #766

Merged
merged 15 commits into from
Jan 17, 2025
Merged

Conversation

dfrg
Copy link
Collaborator

@dfrg dfrg commented Dec 12, 2024

This adds support for image extend modes, alpha and nearest neighbor sampling to fine and plumbs support for all through the pipeline.

Extend modes with bilinear filtering (bottom right is x repeat and y reflect):
image_extend_bilinear

Extend modes with nearest neighbor filtering:
image_extend_nearest

Image alpha:
image_alpha

dfrg added 2 commits December 12, 2024 01:12
This adds support for image extend modes, alpha and nearest neighbor sampling to fine and plumbs support for all through the pipeline.
@dfrg dfrg mentioned this pull request Dec 12, 2024
Copy link
Collaborator

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

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

LGTM. I don't know if we need to quantize the alpha down to 8 bits and store it with the other stuff, but okay for now.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I think this should have a changelog entry.

I'd also like to see some new tests here; these should be very similar to the existing tests, and so should be straightforward. Indeed, one of the tests needs to be updated.

Otherwise, only relatively peripheral concerns.

Cargo.toml Outdated Show resolved Hide resolved
examples/scenes/src/test_scenes.rs Outdated Show resolved Hide resolved
vello_encoding/src/encoding.rs Show resolved Hide resolved
vello_encoding/src/encoding.rs Show resolved Hide resolved
vello_encoding/src/draw.rs Outdated Show resolved Hide resolved
vello_shaders/shader/fine.wgsl Outdated Show resolved Hide resolved
vello_shaders/shader/fine.wgsl Show resolved Hide resolved
vello_shaders/shader/fine.wgsl Show resolved Hide resolved
vello_shaders/shader/fine.wgsl Show resolved Hide resolved
@nicoburns nicoburns added the enhancement New feature or request label Dec 12, 2024
@dfrg
Copy link
Collaborator Author

dfrg commented Dec 13, 2024

Thanks for the feedback, Daniel. I'm on vacation and out of town for the next week but I may have some time over the weekend to address all concerns and get this landed.

@nicoburns nicoburns added this to the Vello 0.4 Release milestone Dec 21, 2024
@waywardmonkeys
Copy link
Collaborator

@dfrg I took the liberty of updating this branch to fix the merge conflicts and a couple of other things. Feel free to force push and remove my changes if you have others locally that you prefer.

@waywardmonkeys
Copy link
Collaborator

Odd that the nearest neighbor snapshot that I generated on my machine is slightly different than the one generated in CI.

@dfrg
Copy link
Collaborator Author

dfrg commented Jan 4, 2025

@dfrg I took the liberty of updating this branch to fix the merge conflicts and a couple of other things. Feel free to force push and remove my changes if you have others locally that you prefer.

Not a problem. Your changes look good. Thanks for pushing this forward.

@dfrg
Copy link
Collaborator Author

dfrg commented Jan 4, 2025

Odd that the nearest neighbor snapshot that I generated on my machine is slightly different than the one generated in CI.

I’ve also noticed inconsistent failures between local tests and CI. I’m not sure how to resolve this properly but I’ll play with this one and see if I can get it to pass as this PR would be nice to land for 0.4.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The border on the left side of the two right images looks off to me. Which platform/GPU did you use to generate these?

Copy link
Collaborator

Choose a reason for hiding this comment

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

M3 Pro Max laptop, macOS 15.2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I’ll test on my M3 in a bit and see if I can reproduce.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So there’s actually a fairly thick border around the entire image and this appears on the previous version as well. Note that this only seems to occur when generating snapshots on macOS. The winit demo is fine on macOS and the snapshots appear as they should on Windows.

I don’t have a lot of time to continue investigating this right now but I’ll come back to it later this week.

Copy link
Collaborator Author

@dfrg dfrg Jan 4, 2025

Choose a reason for hiding this comment

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

Additional data point: on my win11/rtx3070 machine, the COLR snapshot tests fail locally on main even though they pass in CI.

This was linked to issues Jan 8, 2025
@DJMcNab
Copy link
Member

DJMcNab commented Jan 10, 2025

Discussion in office hours yesterday suggests that we should just skip the testing for now.

I don't have a Mac to dig into this further myself. I understand that this does occur consistently on macOS (and so that with_winit doesn't in itself fix it) - Bruce has done some testing which suggests this.
It's only seems to be a one-pixel failure, which won't be a massive issue at realistic screen sizes.
We certainly should track this in an issue, though (and ideally even fix it).

@nicoburns
Copy link
Contributor

Given that this is new functionality (and thus not a regression), I agree that shipping this with a known bug is reasonable. It's the sort of the thing that could likely be fixed in a patch release (if we can work out what's wrong (and it doesn't require a wgpu upgrade I suppose)).

@dfrg
Copy link
Collaborator Author

dfrg commented Jan 10, 2025

I suspect this might be a very slight difference in path rendering leading to over-extension of the image. I’ll do some tests to confirm this but in the meantime, I’m fine with disabling these failing tests and merging.

@dfrg dfrg added this pull request to the merge queue Jan 17, 2025
Merged via the queue into main with commit 0e79178 Jan 17, 2025
17 checks passed
@dfrg dfrg deleted the image-sampling branch January 17, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tiling an image Encode alpha for Images
4 participants