image icon indicating copy to clipboard operation
image copied to clipboard

Remove GenericImage<Pixel = Rgba<u8>> impl for DynamicImage

Open fintelia opened this issue 1 year ago • 2 comments

Revive #2136

Currently the DynamicImage type pretends to be an rgba8 image even when it isn't. This makes it easier to use in places where the pixel format must be statically known, but unfortunately causes silent bugs and user confusion when dynamic images are to store images that aren't rgba8.

This change is likely to be disruptive because there's a lot of user code that currently relies on these trait implementations. But unfortunately I don't see a good alternative. Before the next major release we'll likely need to investigate what downstream code would be impacted and whether we can ease the migration effort.

Fixes #1952, Fixes #2274

fintelia avatar Oct 12 '24 19:10 fintelia

:+1: Agree in principle, yes, this should be gone. If we de-emphasize use of the type system for describing the image, the trait should naturally play a diminished role in future designs anyways. It does not sufficiently generalize operations we want in any case.

As for mitigations: The role of Dynamic* image is that its variants are all convertible in some sense. I wonder if we could provide an explicit as_image::<P: Pixel>() -> TBD<P> adapter which implements the trait variant that the caller chooses. This doesn't resolve the inherent problem we have with the Pixel type itself but resolves that we need to choose one. Just an idea, I'd want to move this PR forward regardless of the evaluation of that idea.

197g avatar Oct 13 '24 01:10 197g

@fintelia Should we just decide to do the cut-off for the next major version now by

  • a 0.25.3 release
  • moving 0.25 into a version branch (not promising any more releases, but who knows)
  • declaring main to be development by marking its version as 0.26-alpha?

It seems like a good time to get rid of old cruft.

197g avatar Oct 13 '24 01:10 197g