Improved transform API

[rfuest]

This was discussed in #162 and #198 before.

If I remember correctly we decided to only implement fn translate(&mut self, offset: Point), because the old behavior can still be accessed using object.clone().translate(offset).

[jamwaffles]

Hit a bit of a snag trying to get a PR going for this issue. The trait should look like this, right?

trait Transform {
    fn translate(&mut self, by: Point) -> &mut Self;
}

This breaks lines like this:

let line = Line::new(start, end).translate(Point::new(10, 10));

let line2 = Line::new(start, end)
    .translate(Point::new(10, 10))
    .into_styled(PrimitiveStyle::with_stroke(BinaryColor::On, 1));

If I change the trait to fn translate(self, by: Point) -> Self then the above do work, but the following breaks (I think because this version copies self instead of moving it?):

let mut line = Line::new(start, end);

line.translate(Point::new(10, 10));

Any ideas? I really want to support both patterns. I can put what I have in a PR for a review if you like.

[rfuest]

This problem was the reason I decided not to to try to implement this in PR #198 and I think there won't be a good solution to support both patterns with one method.

A LineBuilder would make the API cleaner, by separating owned vs. borrowed, but also more verbose:

let line =
    LineBuilder::new(start, end)
    .translate(Point::new(10, 10))
    .build();

 // equivalent to

let mut line = Line::new(start, end);
line.translate(Point::new(10, 10))

let line2 =
    LineBuilder::new(start, end)
    .translate(Point::new(10, 10))
    .build_styled(PrimitiveStyle::with_stroke(BinaryColor::On, 1));

 // equivalent to

let mut line2 = Line::new(start, end);
line2.translate(Point::new(10, 10))
let styled_line2 = line2.into_styled(PrimitiveStyle::with_stroke(BinaryColor::On, 1));

trait Transform {
    fn translate(self, by: Point) -> Self;
}

// translate owns self for LineBuilder
impl Transform for LineBuilder {
    ...
}

// translate borrows self for Line
impl Transform for &mut Line {
    ...
}

[rfuest]

After thinking about this some more, adding a builder which only exists to make primitives translatable seems weird.

Another solution would be a map like function that takes a closure to make translate (and other transforms) chainable:

trait Translate {
    fn translate(&mut self, by: Point);
}

impl Translate for Line { ... }

trait Transform {
    fn transform<F>(self, f: F) -> Self where F: Fn(&mut Self);
}

impl Transform for Line {
    fn transform<F>(mut self, f: F)  -> Self where F: Fn(&mut Self) {
        f(&mut self);
        self
    }
}

// -------------------

let line = Line::new(start, end)
    .transform(|l| l.translate(Point::new(10, 10)));

let line2 = Line::new(start, end)
    .transform(|l| l.translate(Point::new(10, 10)))
    .into_styled(PrimitiveStyle::with_stroke(BinaryColor::On, 1));

let mut line3 = Line::new(start, end);
line3.translate(Point::new(10, 10));

[jamwaffles]

Do we want to put this in 0.6.0 as the last remaining breaking change, or leave the current .translate()/.translate_mut() as is?

[rfuest]

Do you mean leaving this as is for the 0.6.0 release and decide later or to leave it as is forever?

[jamwaffles]

Leave it as is for 0.6.0 and decide for 0.7.0 (or later). 0.7.0 will already get some breaking changes around fonts IIRC so, as it won't be breaking-change free, we could put the transform changes in there.

I'm keen on getting 0.6.0 released which is why I asked :)

[rfuest]

I'm keen on getting 0.6.0 released which is why I asked :)

I agree that it's time to release 0.6.0.

Leave it as is for 0.6.0 and decide for 0.7.0 (or later). 0.7.0 will already get some breaking changes around fonts IIRC so, as it won't be breaking-change free, we could put the transform changes in there.

IMO this issue is related to the other geometry related issues like #156 and #182. These issues are also moved to a later release, so I think it's OK to also handle this later.

[jamwaffles]

IMO this issue is related to the other geometry related issues like #156 and #182.

Yeah good point. This might not happen, but it would be great to have a theme for the breaking changes in the next release. Geometry seems like it might be that theme :)

... so I think it's OK to also handle this later.

Brill :+1:

[rfuest]

The context API proposed in the now closed PR #149 provided an easy way of transforming a group of drawing operations without having to individually transform each item. One way of replicating this function without having to create a completely new way of drawing items would be to introduce a virtual draw target that can wrap an existing draw target:

struct TranslatedDrawTarget<T> {
   target: T,
   offset: Point,
}

impl<T> DrawTarget for TranslatedDrawTarget<T> { ... }

fn main() {
   ...
   let mut translated = TranslatedDrawTarget::new(display, Point::new(10, 20));
   Rectangle::new(...).into_styled(...).draw(&mut translated);
   Circle::new(...).into_styled(...).draw(&mut translated);
}

[jamwaffles]

Do we want to close this issue as stale now, or keep it open as additional functionality to implement? I'm not sure if the original intent is serviced by the new sub-drawtargets or not.

[rfuest]

I'm not sure if the original intent is serviced by the new sub-drawtargets or not.

The transform API is more generic and isn't limited to drawing operations, so IMO we need both.

@ryankurte It would be great if you could give some input on this issue. Me an @jamwaffles couldn't decide how to handle this the best way and another pair of eyes would help.

The TLDR is: We have a Transform trait that is implemented by drawable objects:

pub trait Transform {
    /// Move the origin of an object by a given number of (x, y) pixels, returning a new object
    fn translate(&self, by: Point) -> Self;

    /// Move the origin of an object by a given number of (x, y) pixels, mutating the object
    /// in place
    fn translate_mut(&mut self, by: Point) -> &mut Self;
}

The returned values can be used to chain multiple transformations together: obj.translate(p1).translate(p2).

We are unsure if we need both variants of each transformation methods (additional methods, like rotate, will be added later). Having only one method per transformation would reduce code duplication and make the API smaller.

Using only the mutable variant would be OK for most use cases, but would break this: https://github.com/embedded-graphics/embedded-graphics/issues/211#issuecomment-566769260 One possible solution was suggested here, but I'm not sure if this is any better than having two methods per transform: https://github.com/embedded-graphics/embedded-graphics/issues/211#issuecomment-566898328

[jamwaffles]

Purely to reduce the amount of code duplication, would adding a blanket impl help?

pub struct Point;

pub trait Transform {
    /// Move the origin of an object by a given number of (x, y) pixels, returning a new object
    fn translate(&self, by: Point) -> Self;
}

pub trait TransformMut: Transform + Sized {
    /// Move the origin of an object by a given number of (x, y) pixels, mutating the object
    /// in place
    fn translate_mut(&mut self, by: Point) -> &mut Self {
        *self = self.translate(by);

        self
    }
}

impl<T> TransformMut for T where T: Transform {}

This does require TransformMut: Sized but I don't think that's much of a problem. A blanket impl wouldn't resolve the question of API surface however, but makes the case for retaining both non mutating and mutating transforms slightly stronger.

[ryankurte]

hmm, interesting problem. maybe another situation in which this could be split/reversed? (sorry the suggestion is so much larger than the question...)

at the moment Drawable takes a DrawTarget bound over a colour for drawing, so you call SomeObject.draw(display, ...). what if that was inverted so instead Drawable provides a pixel iterator / getter and instead you call Display.draw(SomeObject)?

then transformations would all be magically generic over anything that could be drawn, basically as proposed above but backwards, this could look something like:

/// Could possibly be an associated type but, could also just be embedded_graphics standard
pub enum DrawError {}

pub struct Point {
    x: u32,
    y: u32,
}

/// Drawable / sprite equivalent trait, isolated from Display, ideally from Colour too, though this _may_
/// need an associated Colour, per #511 this should be find as colour type should be constant
pub trait Drawable {
    /// Fetch size of drawable object
    fn size(&self) -> (u32, u32);
  
    /// Possible draw trait, not 100% sure if C should be dyn but, i _think_ this works
    fn draw<T: FnMut(u32, u32, bool)>(&self, target: T) -> Result<(), DrawError>;
}

/// Transformation wrapper type, applies transforms to drawable reference
pub struct Transformer<'a, D: Drawable> {
    mode: TransformMode,
    d: &'a D,
}

pub enum TransformMode {
    Shift(Point),
    Scale(Point),
    Reflect,
    Flip,
    Invert,
}

impl <'a, D: Drawable> Drawable for Transformer<'a, D> {
    fn size(&self) -> (u32, u32) {
        self.d.size()
    }

    fn draw<T: FnMut(u32, u32, bool)>(&self, mut canvas: T) -> Result<(), DrawError>{
        match &self.mode {
            TransformMode::Shift(shift) => self.d.draw(|x, y, v| (canvas)(x+shift.x, y+shift.y, v)),
            TransformMode::Scale(_scale) => unimplemented!(),   // Left as an exercise to the reader
            TransformMode::Reflect => {
                let r = self.size().0;
                self.d.draw(|x, y, v| (canvas)(r - x, y, v))
            },
            TransformMode::Flip => {
                let f = self.size().1;
                self.d.draw(|x, y, v| (canvas)(x, f - y, v))
            },
            TransformMode::Invert => self.d.draw(|x, y, v| (canvas)(x, y, !v)),
        }
    }
}

/// Transform traits then become generic over Drawable types
pub trait Transform<D: Drawable> {
  fn shift<'a>(d: &'a D, p: Point) -> Transformer<'a, D> {
    Transformer{d, mode: TransformMode::Shift(p)}
  }
  fn scale<'a>(d: &'a D, p: Point) -> Transformer<'a, D> {
    Transformer{d, mode: TransformMode::Scale(p)}
  }
  fn reflect<'a>(d: &'a D) -> Transformer<'a, D> {
    Transformer{d, mode: TransformMode::Reflect}
  }
  fn flip<'a>(d: &'a D) -> Transformer<'a, D> {
    Transformer{d, mode: TransformMode::Flip}
  }
  fn invert<'a>(d: &'a D) -> Transformer<'a, D> {
    Transformer{d, mode: TransformMode::Invert}
  }
}

it'd be a fairly significant overhaul perhaps, and i'm surely overlooking some downsides... but, i think it'd work nicely for #511 too? i think this removes the need for a mutable transform, and a DrawColour trait/decorator could be implemented in the same manner as Transform which could clean up the colour handling a bunch.

[rfuest]

hmm, interesting problem. maybe another situation in which this could be split/reversed? (sorry the suggestion is so much larger than the question...)

Long answers are good as long as they contain interesting information, as yours does. But the question was actually concerning something different (and simpler): We have primitive objects, like Rectangle or Circle. These objects can be used for drawing operations, but also for other uses, like hit testing: circle.contains(touched_point). These objects can be transformed using the Transform trait.

at the moment Drawable takes a DrawTarget bound over a colour for drawing, so you call SomeObject.draw(display, ...). what if that was inverted so instead Drawable provides a pixel iterator / getter and instead you call Display.draw(SomeObject)?

The API used to be display.draw(object) before e-g 0.6. We had changed it to the current implementation to make it easier to implement some support for hardware accelerated drawing operations in the Drawable impls. DrawTargets do not only provide a way to set individual pixels, but also methods to fill rectangles or fill a rectangular area with an iterator of pixel colors. These operations can be much faster if the DrawTarget doesn't have an internal framebuffer, but transfers each pixel directly to an external display controller.

We have added some draw time transformation since this issue was started: https://github.com/embedded-graphics/embedded-graphics/tree/master/src/draw_target These are basically the reversed version of the Transformer you suggested (applies to DrawTargets instead of Drawables).

/// Could possibly be an associated type but, could also just be embedded_graphics standard
pub enum DrawError {}

The error types are currently dictated by the DrawTarget implementation. For display drivers that use a framebuffer drawing operations can never fail, so that they use Infallible as an error type. If Rust will provide special support for infallible errors in the future it might be worth to keep the errors as an associated type. But using a common error type might simplify some things. Many display drivers have switch to using display-interface crate, which has an error type that we could probably reuse: https://github.com/therealprof/display-interface/blob/cfdaae3f8f1ed0bc320a49ec494a17dbd44c40e6/src/lib.rs#L16-L31

[bugadani]

All right, this isn't Transform-specific, but I burnt myself with this today. What would you think about sprinking #[must_use] around rather liberally? I was chasing a bug in embedded-layout where I called Transform::translate instead of Transform::translate_mut and I #[must_use] could have saved me 20 minutes.

[rfuest]

That's a good idea. Can you open a separate issue for this suggestion, because it doesn't only apply to the transformation API?