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

Use Rectangle for Dimensions trait #312

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

rfuest
Copy link
Member

@rfuest rfuest commented Apr 6, 2020

Hi! Thank you for helping out with Embedded Graphics development! Please:

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

PR description

This PR changes the Dimensions trait to return a Rectangle instead of providing separate methods for getting the corners and size. This reduces code duplication and makes it easier to get consistent behavior that should match #182.

One example of how this PR can simplify code is the change in simulator/src/framebuffer.rs. It uses the new Points iterator to iterate over all pixels inside the bounding box.

The Dimension implementation for Circle was broken before this PR and wasn't fixed in the PR to keep the scope of this PR smaller. I'll open another PR after this one is merged.

}
for p in item.primitive.bounding_box().points() {
if let Some(pixel) = self.get_pixel_mut(p) {
pixel.copy_from_slice(color);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😚👌

That's delightful :)


fn size(&self) -> Size {
self.primitive.size()
//FIXME: The bounding box for a styled primitive should use the stroke width and alignment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're not going to fix this soon, can you create an issue so it doesn't get forgotten about please?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be fixed soon, but I've started a tracking issue for these changes.

Comment on lines +91 to +92
//FIXME: This temporary method replicates the old broken conversion from diameter to size and
//should be removed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the other comment, please open an issue if this won't be fixed in the immediate future.

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from a couple of comments this looks good.

@rfuest rfuest mentioned this pull request Apr 7, 2020
15 tasks
@rfuest rfuest merged commit d91c813 into embedded-graphics:master Apr 7, 2020
@rfuest rfuest deleted the bounding_box branch July 20, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants