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

refactor: ellipse rasterizer #692

Merged
merged 1 commit into from
Jun 26, 2022

Conversation

marco-langer
Copy link
Contributor

@marco-langer marco-langer commented Jun 26, 2022

Description

This PR implements some of the mentioned issues in my other comment regarding the ellipse rasterizer, namely:

  • renamed midpoint_elliptical_rasterizer to midpoint_ellipse_rasterizer
  • replaced colour vector with pixel type
  • replaced std::array with point type for center and semi-axis
  • removed unnecessary deep copies of temporaries.
  • removed printing warnings to cout

There are still some open questions, e.g. why circle/line and ellipse rasterizer have different signedness for point types and why they implement the call operator differently. However, I am not sure if we are able to find any consensus on these topics before the deadline for Boost 1.80.

Tasklist

  • Ensure all CI builds pass
  • Review and approve

@codecov
Copy link

codecov bot commented Jun 26, 2022

Codecov Report

Merging #692 (7279ffa) into develop (151fd9c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #692   +/-   ##
========================================
  Coverage    80.32%   80.32%           
========================================
  Files          117      117           
  Lines         5032     5032           
========================================
  Hits          4042     4042           
  Misses         990      990           

@mloskot mloskot added ext/rasterization boost/gil/extension/rasterization cat/refactoring Any nonfunctional changes labels Jun 26, 2022
Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

Thank you very much!

Since @meshtag reacted to your questions in #585 (comment) and he did not express any objections, I'm happy to approve it and merge it for Boost 1.80

@mloskot mloskot added this to the Boost 1.80 milestone Jun 26, 2022
@mloskot mloskot merged commit adddbec into boostorg:develop Jun 26, 2022
@mloskot mloskot mentioned this pull request Jun 26, 2022
3 tasks
Copy link
Member

@meshtag meshtag left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks a lot @marco-langer and @mloskot !

mloskot added a commit that referenced this pull request Jun 27, 2022
* develop:
  docs!: Announce plan to require C++17 after Boost 1.80 (#694)
  feat: Added apply_rasterizer() free function (#695)
  refactor: Ellipse rasterizer according to the comment at (#692)
  refactor: Deprecate apply_operation in favor of variant2::visit for any_image (#656)
  refactor: Replace deprecated libtiff v4.3 typedefs with C99 fixed-size integers (#685)
  fix: Automatic detection of <filesystem> header (#684)
  test: Add tiled TIFF test case to simple_all_formats
  test: Add tests for RGB to HSL (#691)
  refactor: Move RGB to HSL tests to color_convert_rgb.cpp
  refactor: Make with_tolerance reusable across other tests
  chore: Correct include guard
  fix: Add missing #include <array>
  fix: Wrong RGB -> HSL convertion (#505)
@mloskot mloskot mentioned this pull request Jul 5, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/refactoring Any nonfunctional changes ext/rasterization boost/gil/extension/rasterization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants