Add sample/interpolate nearest #1962
Conversation
| if !(0.0..=((w - 1) as f32)).contains(&x) { | ||
| return None; | ||
| } | ||
| if !(0.0..=((h - 1) as f32)).contains(&y) { | ||
| return None; | ||
| } | ||
|
|
||
| Some(img.get_pixel(x.round() as u32, y.round() as u32)) |
There was a problem hiding this comment.
I think we should be rounding before checking whether the index is in bounds. For instance, (-0.3, -0.3) should round to (0,0) and thus be accepted
There was a problem hiding this comment.
If that's the case then, I think that we should also do the same for bilinear interpolation, otherwise it would lead to inconsistent boundary conditions.
Not that we have to go with the convention of glsl, but for reference
https://www.khronos.org/opengl/wiki/Sampler_Object#:~:text=If%20GL_NEAREST%20is%20used%2C%20then,between%20the%20nearest%20adjacent%20samples.
If a user were to implement wrapping outside of this, then -0.3 could be considered as both 0 and w-1
There was a problem hiding this comment.
Actually, I'm realizing that sample_linear is already inconsistent. Bilinear sampling at uv coordinates (0,0) should produce the top left pixel's color if doing "clamp-to-edge" and should be the average of the four corner pixels for "wrapping".
Specifically, the ui.max(0.).min((w - 1) as f32) implements the "clamp to edge".
There was a problem hiding this comment.
you mean inconsistent with interpolate_bilinear or sample_nearest?
It only clamps to edge to handle numerical inconsistencies, but both methods should return None if it's out of bounds.
There was a problem hiding this comment.
I mean that sample_linear already can't be used to implement wrapping sample, since not matter what arguments the user calls it with, they won't be able to interpolate between pixels on opposite sides of the image.
There was a problem hiding this comment.
That's true... One alternative is to provide a separate function but that seems not as clean (akin to wrapping add, saturating, checked).
There was also providing an enum as an option, which forces the user to think about it, but may be unfriendly to people learning the library.
I looked at how Vulkan handles sampling, and they have an object with a number of settings to sample from an image. Maybe it makes sense to attach these to an object which has sensible defaults (clamp to border), but can be changed if the user needs precise control. It will also be easier to modify in the future from a backcompat perspective if new features need to be added.
JulianKnodt
force-pushed
the
sample_coord
branch
from
a84ae8e
to
d1c7ec5
Compare
This looks very similar to the prior PR, but I'm not sure if the two sample modes should be merged into one?
That was originally why I had included an enum to indicate which to perform, but it's fine to keep them separate as well.