Skip to content

Conversation

LikeLakers2
Copy link

@LikeLakers2 LikeLakers2 commented Sep 10, 2025

This PR was made in response to #2599 (comment).

This PR contains a breaking change, and thus should not be included until the next major version.

This changes the function signature of crate::imageops::filter3x3() (and its accompanying function in DynamicImage). Rather than take a &[f32] as before, now it takes a &[f32; 9], enforcing that the kernel provided is of length 9 in a much more user-friendly way.

Notably, not every user of filter3x3() will need to explicitly convert the kernel they provide. For example, one test in image contains this line, which didn't need to be changed:

&|img| img.filter3x3(&[0.0, -1.0, 0.0, -1.0, 5.0, -1.0, 0.0, -1.0, 0.0]),

But for those that do, the TryFrom<&[T]> for &[T; N] impl (provided by the Rust standard library) will fit most (if not all) cases. slice::as_array() is also available if the user is on nightly.

In summary:

  • crate::imageops::filter3x3() no longer assumes the kernel is of length 9 (it appears to have done so before, without giving any form of error)
  • DynamicImage::filter3x3() now errors at compile-time if given the wrong length, rather than checking at run-time and stopping the program there.
  • Some users of filter3x3() may need to modify their code to handle the new type signature. However, in most cases the solution is simple, and provided by the Rust standard library.

@LikeLakers2
Copy link
Author

LikeLakers2 commented Sep 10, 2025

RE: Clippy workflow failing https://github.com/image-rs/image/actions/runs/17609577161/job/50028151587?pr=2600

Looking at the logs, it appears Clippy is generating errors on files that this PR does not touch. As such, I won't be fixing those errors in this PR.

@197g 197g added the next: breaking Information tag for PRs and ideas that require an interface break label Sep 10, 2025
@197g
Copy link
Member

197g commented Sep 10, 2025

The clippy warning is also a false positive, it seems to think colors[0][i] and colors[1][i] means that 'i is only indexing colors' instead of realizing it indexes separate places. They are aware: rust-lang/rust-clippy#15486 (comment)

@LikeLakers2
Copy link
Author

LikeLakers2 commented Sep 12, 2025

It does occur to me, some users may have been relying on crate::imageops::filter3x3() allowing shorter kernel sizes? It didn't error out before, after all.

But as I don't use filter3x3() myself, I don't know if providing a kernel with less than array length 9 should've been considered erroneous in the first place.

At the very least, I don't know if there's a way to replicate the same results as shorter kernel sizes, short of copying the function wholesale.

@197g
Copy link
Member

197g commented Sep 12, 2025

That would be rather modest advantages when the fixed size array can be copied to a stack place where it is extended. Maybe from a performance point of view where the zip is shorter? But then it is really the implementation which should be improved to skip any position with a zeroed coefficients entirely, by pre-processing the kernel and zip into a single structure. I don't think that should influence the interface type decision much.

@LikeLakers2
Copy link
Author

LikeLakers2 commented Sep 12, 2025

@197g, I'm not quite sure I understood all that (sorry!), so I have to ask: Is that response saying that this PR is fine as-is?

@197g
Copy link
Member

197g commented Sep 12, 2025

Yes, the PR is fine as is.

No worries, better asking once more for clarification than a misunderstanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

next: breaking Information tag for PRs and ideas that require an interface break

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants