-
-
Notifications
You must be signed in to change notification settings - Fork 24
Add support for non-square pixels #71
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
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this function up to be directly below scale.
| ### Added | ||
|
|
||
| - Added support for non-square pixels | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ### 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately CI isn't happy with one of the suggestions I made.
| pub theme: BinaryColorTheme, | ||
| } | ||
|
|
||
| #[cfg(feature = "with-sdl")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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