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

Add sampling using [0,1] for GenericImageView #1929

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

JulianKnodt
Copy link
Contributor

WIP
Not sure whether this should directly go on GenericImageView, and where the enums should go.
Also need to add tests but put the general idea up for review.


Adds a way to sample images in [0,1], which is a common use case in graphics.

Currently adds non-exhaustive enums based on OpenGL's that allow for simple sampling of nearby coordinates.

Nearest sampling can produce artifacts depending on the sampling resolution of the UV whereas bilinear will smoothly interpolate.

Addresses #1905

Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

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

Looking at these changes, I think adding bilinear sampling to this crate is valuable but I worry that a generic sample method is going to add too much complexity. There's just so many different ways to sample an image with a (f32, f32). For instance, there's the question of whether (0,0) is the center of the top left pixel (like in this PR) or the corner of that pixel (like the usual convention in OpenGL/Vulkan/etc.) and that's on top of questions about wrapping mode and filtering method. And no single option is "right"

What would you think of adding just a bilinear_sample method that always clamped to edge and used the same coordinates as get_pixel (ie from 0 to size-1 with integer values being pixel centers). It shouldn't be too hard for users to implement wrapping or normalized coordinates based on that if they want

@JulianKnodt
Copy link
Contributor Author

JulianKnodt commented May 18, 2023

I think that's definitely a step forward.
I do think that being able to switch between different sampling modes is very convenient, so I think keeping the SampleKind is good, and would let additional sample kinds be added more easily in the future.

I think that clamping against the edge quietly is probably going to lead to unexpected behavior for users. Instead, we should probably return an Option<Pixel>, and that will be immediately clear to the user that they've sampled out of bounds, as long as we guarantee that all values in [0,1] do not return None.

@JulianKnodt
Copy link
Contributor Author

Just wanted to bump this, while I am pushing back on some ideas you suggested, if you feel strongly about them I will change it. I would rather have sampling in some form than no sampling.

@fintelia
Copy link
Contributor

fintelia commented Jul 2, 2023

Let's go with one method per supported sampling type. If users want to dynamically pick between sampling types, they can implement it themselves, but this way there's no extra branching if the sampling type is known in advance.

Thinking about it, the methods probably belong in the imageops module with all the other methods implemented for GenericImage[View]

@JulianKnodt
Copy link
Contributor Author

Sounds good, it may take me a bit to implement but I'll add them in.

@JulianKnodt JulianKnodt force-pushed the sample_coord branch 4 times, most recently from d17d8fc to 2945419 Compare July 3, 2023 06:34
src/imageops/sample.rs Outdated Show resolved Hide resolved
src/imageops/sample.rs Outdated Show resolved Hide resolved
src/imageops/sample.rs Show resolved Hide resolved
src/imageops/sample.rs Outdated Show resolved Hide resolved
src/imageops/sample.rs Outdated Show resolved Hide resolved
src/imageops/sample.rs Outdated Show resolved Hide resolved
src/imageops/sample.rs Outdated Show resolved Hide resolved
@JulianKnodt JulianKnodt force-pushed the sample_coord branch 3 times, most recently from b4a651e to 08afcc6 Compare July 4, 2023 06:43
@JulianKnodt JulianKnodt force-pushed the sample_coord branch 2 times, most recently from a3e8dc2 to 575819e Compare July 11, 2023 05:45
@JulianKnodt
Copy link
Contributor Author

JulianKnodt commented Jul 11, 2023

Cheers, was out of town but have fixed most of the comments

@JulianKnodt JulianKnodt force-pushed the sample_coord branch 2 times, most recently from 843777d to 2f4eb9e Compare July 11, 2023 07:41
src/imageops/sample.rs Outdated Show resolved Hide resolved
Adds a way to sample images in [0,1], which is a common use case in graphics.

Currently adds non-exhaustive enums based on OpenGL's that allow for simple sampling of nearby
coordinates.

Nearest sampling can produce artifacts depending on the sampling resolution of the UV
whereas bilinear will smoothly interpolate.
@JulianKnodt
Copy link
Contributor Author

@fintelia You're gonna have to merge it, I don't have the ability to do so

@fintelia fintelia merged commit 2a5c5bf into image-rs:master Jul 17, 2023
92 of 96 checks passed
@JulianKnodt JulianKnodt deleted the sample_coord branch July 17, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants