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.
| 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.
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: