-
-
Notifications
You must be signed in to change notification settings - Fork 116
Make several geometric primitive operations computable in const-eval #794
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
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.
Thanks, more const functions are always nice to have.
The MSRV CI test fails at the moment, but it should be possible to refactor that code without having to raise the MSRV.
|
|
||
| /// Creates a size from two corner points of a bounding box. | ||
| pub(crate) const fn from_bounding_box(corner_1: Point, corner_2: Point) -> 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.
Examples should have a # Examples header:
| /// | |
| /// | |
| /// # Examples | |
| /// |
Edit: I just noticed that the other examples in this file are also missing this header, but let's at least do it right in the new code.
| - Made the following methods `const`: | ||
| - `AnchorPoint::from_xy` | ||
| - `AnchorPoint::x` | ||
| - `AnchorPoint::y` | ||
| - `Point::component_min` | ||
| - `Point::component_max` | ||
| - `Size::component_min` | ||
| - `Size::component_max` | ||
| - `Rectangle::with_corners` | ||
| - `Rectangle::center` | ||
| - `Rectangle::bottom_right` | ||
| - `Rectangle::contains` | ||
| - `Rectangle::intersection` | ||
| - `Rectangle::envelope` | ||
| - `Rectangle::resized` | ||
| - `Rectangle::resized_width` | ||
| - `Rectangle::resized_height` | ||
| - `Rectangle::offset` | ||
| - `Rectangle::anchor_point` | ||
| - `Rectangle::anchor_x` | ||
| - `Rectangle::anchor_y` |
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.
Move these into the Changed section and add a link to the PR.
| ## [Unreleased] - ReleaseDate | ||
|
|
||
| ### Added | ||
| - Made `Size::from_bounding_box` public |
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.
The CHANGELOG should reflect API changes and from a user perspective it doesn't matter that this function has been available internally before:
| - Made `Size::from_bounding_box` public | |
| - [#794](https://github.com/embedded-graphics/embedded-graphics/pull/794) Added `from_bounding_box` constructor to `Size`. |
|
|
||
| ## [Unreleased] - ReleaseDate | ||
|
|
||
| ### Added |
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.
There's already an Added section below.
| /// | ||
| /// assert_eq!(bounded, Size::new(310, 230)); | ||
| /// ``` | ||
| pub const fn from_bounding_box(corner_1: Point, corner_2: Point) -> 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.
There has been some discussion about the name of this method when it was added, but at that time we didn't really care because it was internal only. I don't like the current name, because I wouldn't expect a function that is called from_BOUNDING_BOX take points as parameters and not a Rectangle. But I cannot really think of anything better at the moment and perhaps I'm overthinking this.
Another option would be to not make this method public and instead to make Rectangle::with_corners const. In my opinion Rectangle::with_corners(top_left, bottom_right).size is more readable than Size::from_bounding_box(top_left, bottom_right). And in case it is used in a const context the possible overhead from also calculating Rectangle::top_left doesn't matter.
|
|
||
| ## [Unreleased] - ReleaseDate | ||
|
|
||
| ### Added |
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.
All changes in this PR also affect embedded-graphics-core, which has a separate changelog that also needs to be updated.
Thank you for helping out with embedded-graphics development! Please:
CHANGELOG.mdentry in the Unreleased section under the appropriate heading (Added, Fixed, etc) if your changes affect the public APIrustfmton the projectjust 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
This PR makes it possible to do several common-sense geometry functions in const-eval. It does not use any nightly features, instead I added some polyfills that could easily be removed if const traits were ever to theoretically be stabilized. I tried to keep it simple, and since it is going from non-const to const in the same MSRV, it is most likely not a breaking change. However, I also decided to make the
Size::from_bounding_boxfunction public, because I have some code in other projects that can be greatly simplified with that, and it made sense to do. I am most likely not alone in that situation, as many small displays use caset/paset commands that use values like those.