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

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fintelia
Copy link
Contributor

@fintelia fintelia commented Oct 12, 2024

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

@HeroicKatora
Copy link
Member

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

@HeroicKatora
Copy link
Member

HeroicKatora commented Oct 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants