Add support for non-square pixels#71
Conversation
|
Thanks for the PR. This is a useful addition, but I would like to change the way it is implemented. If this setting is named pixel aspect ratio I would expect |
d42ebfd to
c513b3a
Compare
|
Hello, |
rfuest
left a comment
There was a problem hiding this comment.
That's better. Some of the code can be simplified by using the component wise methods that Point and Size implement.
src/display.rs
Outdated
| let width = self.size.width.saturating_mul(output_settings.scale.width) | ||
| + self.size.width.saturating_sub(1) * output_settings.pixel_spacing; | ||
| let height = self | ||
| .size | ||
| .height | ||
| .saturating_mul(output_settings.scale.height) | ||
| + self.size.height.saturating_sub(1) * output_settings.pixel_spacing; | ||
|
|
||
| Size::new(width, height) |
There was a problem hiding this comment.
This can be simplified by using Size::component_mul:
| let width = self.size.width.saturating_mul(output_settings.scale.width) | |
| + self.size.width.saturating_sub(1) * output_settings.pixel_spacing; | |
| let height = self | |
| .size | |
| .height | |
| .saturating_mul(output_settings.scale.height) | |
| + self.size.height.saturating_sub(1) * output_settings.pixel_spacing; | |
| Size::new(width, height) | |
| self.size.component_mul(output_settings.scale) | |
| + self.size.saturating_sub(Size::new_equal(1)) * output_settings.pixel_spacing |
| .unwrap(); | ||
|
|
||
| if output_settings.scale == 1 { | ||
| if output_settings.scale == Size::new(1, 1) && output_settings.pixel_spacing == 0 { |
src/output_image.rs
Outdated
| let pitch_x = (output_settings.scale.width + output_settings.pixel_spacing) as i32; | ||
| let pitch_y = (output_settings.scale.height + output_settings.pixel_spacing) as i32; |
There was a problem hiding this comment.
You've added a getter for the pitch:
| let pitch_x = (output_settings.scale.width + output_settings.pixel_spacing) as i32; | |
| let pitch_y = (output_settings.scale.height + output_settings.pixel_spacing) as i32; | |
| let pitch = output_settings.pixel_pitch(); |
src/output_image.rs
Outdated
| self.fill_solid( | ||
| &Rectangle::new(p * pixel_pitch + position, pixel_size), | ||
| &Rectangle::new( | ||
| Point::new(p.x * pitch_x, p.y * pitch_y) + position, |
There was a problem hiding this comment.
| Point::new(p.x * pitch_x, p.y * pitch_y) + position, | |
| p.component_mul(pitch) + position, |
src/output_settings.rs
Outdated
| /// Sets the pixel scale to a non-square value. This is useful for displaying | ||
| /// non-square pixels. | ||
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if `width` or `height` is `0`. | ||
| pub fn scale_non_square(mut self, width: u32, height: u32) -> Self { |
There was a problem hiding this comment.
In embedded-graphics related crates it's preferred to take a Size argument instead of separate width and height. The doc comment should start with a single sentence.
| /// Sets the pixel scale to a non-square value. This is useful for displaying | |
| /// non-square pixels. | |
| /// | |
| /// # Panics | |
| /// | |
| /// Panics if `width` or `height` is `0`. | |
| pub fn scale_non_square(mut self, width: u32, height: u32) -> Self { | |
| /// Sets a non-square pixel scale. | |
| /// | |
| /// This is useful for simulating a display with a non-square pixel aspect ratio. | |
| /// | |
| /// # Panics | |
| /// | |
| /// Panics if `width` or `height` is `0`. | |
| pub fn scale_non_square(mut self, scale: Size) -> Self { |
src/output_settings.rs
Outdated
| /// Sets the pixel scale to a non-square value. This is useful for displaying | ||
| /// non-square pixels. | ||
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if `width` or `height` is `0`. | ||
| pub fn scale_non_square(mut self, width: u32, height: u32) -> Self { |
There was a problem hiding this comment.
Please move this function up to be directly below scale.
| ### Added | ||
|
|
||
| - Added support for non-square pixels | ||
|
|
There was a problem hiding this comment.
| ### Added | |
| - Added support for non-square pixels | |
| ### Added | |
| - **(breaking)** [#71](https://github.com/embedded-graphics/simulator/pull/71) Added support for non-square pixels. | |
| ### Fixed | |
| - [#71](https://github.com/embedded-graphics/simulator/pull/71) Fixed pixel spacing for unscaled outputs. |
|
Hello, I applied the required changes |
rfuest
left a comment
There was a problem hiding this comment.
Unfortunately CI isn't happy with one of the suggestions I made.
src/output_settings.rs
Outdated
| @@ -16,12 +16,14 @@ pub struct OutputSettings { | |||
There was a problem hiding this comment.
This attribute causes the build without the with-sdl flag to fail, because pixel_pitch is now used with and without SDL support enabled. I would suggest to just remove the cfg attribute, and instead add an #[allow(unused)] attribute to output_to_display method.
|
updated and tested with cargo build --no-default-features |
Add support for non-square pixels, using fractional numbers
for example, the display I'm working rn has a 2:3 pixel aspect ratio, this makes the simulator realistic to the device