Better image sampling #722
Conversation
| @@ -1761,4 +1763,25 @@ mod impls { | |||
| std_dev, | |||
| ); | |||
| } | |||
|
|
|||
| pub(super) fn image_sampling(scene: &mut Scene, params: &mut SceneParams) { | |||
There was a problem hiding this comment.
We might as well make this a test as well.
vello_shaders/shader/fine.wgsl
Outdated
| if all(atlas_uv < atlas_extents) && area[i] != 0.0 { | ||
| let uv_quad = vec4(max(floor(atlas_uv), image.atlas_offset), min(ceil(atlas_uv), atlas_extents)); | ||
| let uv_quad = vec4(max(floor(atlas_uv), image.atlas_offset), min(ceil(atlas_uv), atlas_max)); |
There was a problem hiding this comment.
Something about this still feels suspicious to me. This seems like it might extend the bounds of the image, but clipped to the value on the outside. That is, imagine the point where:
atlas_extents: vec2(10, 10)
atlas_uv: vec2(9.5, 5)
Then the final uv_quad would be (9, 5). That's true for the entire section where atlas_uv is in (9, 10].
I think the fix (if my analysis is correct) would be to change atlas_extents on line 1169 with atlas_max (and possibly then inlining atlas_extents as it is otherwise unused)
There was a problem hiding this comment.
This does seem funky but I believe it is correct.
Think of it like accessing an array where your guard is len but you don’t want to index beyond len - 1 which is basically what’s happening here. This is slightly different because we’re dealing with floating point so you can end up with values between those two bounds but it ends up working due to the constraints and the lerps. Consider a 10x1 image where our x coordinate for this pixel is 9.5: we’ll end up sampling at x=9 twice and then lerping by 0.5 which is a nop and produces the correct color.
Note that if you implement your suggestion above (replacing extents with max in the bound), it’ll never sample from the right and bottom edges of the image. It seems like replacing the < with <= should fix this but it doesn’t work in practice because, you know, floating point :)
There was a problem hiding this comment.
Thinking about this a bit more, the extents guard really only implements a clamping extend mode which we don’t actually support. And it only clamps on the right/bottom sides. I believe applying a brush transform with a positive translation would generate padding behavior on the left/top sides with the current code.
Since we only support pad, reflect and repeat, we should probably just remove that condition— if the user wants to avoid padding, they should apply the image to geometry that exactly matches the image size.
There was a problem hiding this comment.
Right. I broadly agree with that final conclusion. I think my original suggestion that it was suspicious was right, but my suggested fix was working on clamp, which doesn't currently exist.
|
@dfrg can I adopt this PR to get it over the finish line? |
|
If you have the bandwidth, please feel free. I do have the fine code to handle extend modes and nearest neighbor sampling but it’s not plumbed through. Would you like me to push that to this branch? |
|
If it's not plumbed through yet, I think a follow-up draft PR would be better. But if you want to make that PR now, and let me (or whoever) do the rebase, that would be fine by me. |
|
I’m not by a computer at the moment so if you’d like to just push this one through, I’ll send a follow up later. |
|
@dfrg requesting your review on the GPU side code. I'll wire up the tests tomorrow. |
|
Removal of clamp LGTM but Raph was fairly insistent on only doing texture loads when |
Constrains image sampling to the actual atlas region for a given image and adds a half pixel adjustment to sample from the pixel center.
Addresses #719 and maybe #656
Example 2x2 image with red, blue, cyan and magenta pixels: