Implement DrawTarget for &mut T: DrawTarget

[ivmarkov]

Thank you for helping out with embedded-graphics development! Please:

• Check that you've added passing tests and documentation
• Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, etc) if your changes affect the public API
• Run rustfmt on the project
• Run just build (Linux/macOS only) and make sure it passes. If you use Windows, check that CI passes once you've opened the PR.

PR description

Is there any reason why DrawTarget is not implemented for &mut T where T: DrawTarget?
The major inconvenience of the above is that Clipped, Translated, Cropped and - overall - the DrawTargetExt decorator decorates only &mut DrawTarget references, and not generic DrawTarget instances where these can be either moved or passed by a mutable reference.

As a result, I cannot fulfill the simple use case of creating a display driver, then decorating it and finally moving it into a generic drawing code that needs to own the display. Passing the display to the drawing code by &mut reference is simply not an option for a variety of reasons.

Here's a concrete use case:

• Creating a Display driver
• Decorating it - say - using Cropped - as the display is weird and does not start at (0, 0), nor does it report correct Dimensions bounding box
• Decorating it - say - using ColorConverted so that my drawing code always use a fixed, custom Color color and is thus display color independent

Sure, implementing &mut T where T: DrawTarget also means implementing &mut T and &T for Dimensions and OriginDimensions, but that's OK. What is not so ideal is that once these are implemented, the implementation of Dimensions for T: OriginDimensions conflicts and should be removed, but the question is what is the better compromise:

• Option 1: (What I suggest) - implement DrawTarget for &mut T (similarly to the traits in embedded-hal and the Read / Write traits in STD and allow the user to decorate DrawTarget instances by move or by &mut reference)
• Option 2: Stay with the status quo, where Dimensions is auto-implemented for types implementing OriginDimensions, but then DrawTarget decoration capabilities of the library are severely limited

With Option 1, there is the extra benefit, that Drawable no longer needs to take &mut DrawTarget and can just switch to plain DrawTarget.

[ivmarkov]

Just to mention that the CI fails as the code is of course not completely backwards compatible: display drivers that used to only implement OriginDimensions now need to explicitly implement Dimensions as well, which is annoying. But then... it boils down to choosing Option 1 (and asking downstream to update) or staying with Option 2.

[rfuest]

Thanks for the PR. I had tried to implement this before and also encountered the problem with the OriginDimensions trait. At the time I had abandoned the idea, because there we hadn't plan any other breaking changes to the driver API. But there will be some breaking changes soon and this PR should be part of it IMO.

[ivmarkov]

Thanks for the PR. I had tried to implement this before and also encountered the problem with the OriginDimensions trait. At the time I had abandoned the idea, because there we hadn't plan any other breaking changes to the driver API. But there will be some breaking changes soon and this PR should be part of it IMO.

Thanks for the feedback. Please let me know when/if you would need my assistance to upstream this PR.
In the meantime, I've published this micro-crate - which - amongst other things - is also providing an "owned" version of the DrawTarget transformations (clipped, cropped, translated and so on).

Might be useful to other folks who hit the same issue.

As you can see, the code ain't pretty and even needs lifetime GATs, hence this PR.

[rfuest]

Perhaps we should just get rid of the OriginDimensions trait entirely. If I remember correctly, the only place where it is really required is the ImageDrawable trait, but we could also add a size method to ImageDrawable.

[rfuest]

Just a stylistic choice, but IMO using the T type explicitly makes these statements easier to understand:

Suggested change

	(**self).draw_iter(pixels)
	T::draw_iter(self, pixels)

[ivmarkov]

I don't really have a strong preference here, so I'm fine with that.

[rfuest]

Code like this will be probably be pretty common and I think we should make it a bit nicer:

Suggested change

	let mut clipped = (&mut display).clipped(&area);
	let mut clipped = display.by_ref().clipped(&area);

This could be implemented by adding a by_ref method to the DrawTarget trait, like in the std::io::Read trait https://doc.rust-lang.org/src/std/io/mod.rs.html#841-881.

[ivmarkov]

Ditto. Let me make those changes in the next days. Perhaps the key question is what we have to do w.r.t. Dimension and OriginDimension from above.

[ivmarkov]

Perhaps we should just get rid of the OriginDimensions trait entirely. If I remember correctly, the only place where it is really required is the ImageDrawable trait, but we could also add a size method to ImageDrawable.

Wouldn't it be simpler if we actually do the reverse:

• Get rid of Dimensions
• Have a hard requirement that all DrawTarget instances actually implement OriginDimensions
• Rename OriginDimensions to Dimensions

That means, we should retire the translated DrawTarget transformation, and clipped should also do what cropped does (moving the top-left corner to (0, 0), but the net result might be, that user code would be just much simpler, as the screen / draw target origin would then always start at (0, 0).

And yes, there are "odd" displays like a TTGO ESP32 board I've seen where the top-left corner is not (0, 0), but I would then say, this is the duty of the driver to do the necessary crop-ing so that the user-visible origin is at (0, 0).

[rfuest]

Wouldn't it be simpler if we actually do the reverse:

* Get rid of `Dimensions`
* Have a hard requirement that all `DrawTarget` instances actually implement `OriginDimensions`
* Rename `OriginDimensions` to `Dimensions`

That means, we should retire the translated DrawTarget transformation, and clipped should also do what cropped does (moving the top-left corner to (0, 0), but the net result might be, that user code would be just much simpler, as the screen / draw target origin would then always start at (0, 0).

I'm not sure I want to loose the flexibility of allowing DrawTargets to use a custom coordinate system. With the current system, you could for example have a DrawTarget with the origin in the center of the display. But I agree that always having the DrawTarget start at (0, 0) could also simplify some code. I'll need to think about this some more. (@jamwaffles: what is your opinion?)

And yes, there are "odd" displays like a TTGO ESP32 board I've seen where the top-left corner is not (0, 0), but I would then say, this is the duty of the driver to do the necessary crop-ing so that the user-visible origin is at (0, 0).

Yes, this should be handled by the display driver.

[ivmarkov]

I'm not sure I want to loose the flexibility of allowing DrawTargets to use a custom coordinate system. With the current system, you could for example have a DrawTarget with the origin in the center of the display.

Strong argument, I agree. In any case, we are aligned on getting rid of either Dimensions or OriginDimensions. Two are too many?

[rfuest]

Strong argument, I agree. In any case, we are aligned on getting rid of either Dimensions or OriginDimensions. Two are too many?

I think so. While I did liked the idea of having the type level guarantee that the top left corner of a draw target is at (0, 0), by implementing OriginDimensions, I'm not sure if it provided any actual benefits.

Another argument for keeping the Dimensions trait, that I forgot yesterday, is that it is also used in other parts of e-g to get the bounding box of primitives and text.

[jamwaffles]

I'm not sure I want to loose the flexibility of allowing DrawTargets to use a custom coordinate system. (@jamwaffles: what is your opinion?)

Thanks for asking :). I agree with this - I think it's really useful to be able to massage the coordinate system. If we see a native UI library built on e-g at some point, one motivating use case for this feature is being able to take sub-regions of the display to lay out sub-pieces of the UI with minimal maths.

I'd be ok seeing OriginDimensions go, but perhaps we should add a Rectangle::with_size(size: Size) -> Self method to make porting code to Dimensions a bit easier and/or less error prone?

[rfuest]

... I'd be ok seeing OriginDimensions go, ...

OK, great to see that we all seem to agree that just removing OriginDimensions is the way to go.

...but perhaps we should add a Rectangle::with_size(size: Size) -> Self method to make porting code to Dimensions a bit easier and/or less error prone?

Yes, I think this would help. Or we could use this: #448 (comment)

[jamwaffles]

Or we could use this

Nice find from 2020! Size::new(self.width, self.height).to_rectangle() (or similar) does look nice.