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

feat: added apply_rasterizer() free function #695

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

marco-langer
Copy link
Contributor

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

Description

This PR implements the gil::apply_rasterizer() free function mentioned in #680.

However, I had to change the API of all three rasterizers quite considerably in order to squeeze them through this common interface. I am not sure if all those API changes are still debatable if we consider the deadline for Boost 1.80 in three days on 29th of June.

If you look at the updated examples, the usability is improved considerable (in my opinion) and it is still worth it (in my opinion) to discuss the PR, but I also understand that it might be too late for these kind of major API changes.

References

#680

Tasklist

  • Ensure all CI builds pass
  • Review and approve

@codecov
Copy link

codecov bot commented Jun 26, 2022

Codecov Report

Merging #695 (0fd05dc) into develop (adddbec) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #695      +/-   ##
===========================================
+ Coverage    80.69%   80.75%   +0.05%     
===========================================
  Files          116      116              
  Lines         5072     5086      +14     
===========================================
+ Hits          4093     4107      +14     
  Misses         979      979              

@mloskot
Copy link
Member

mloskot commented Jun 27, 2022

I am not sure if all those API changes are still debatable if we consider the deadline for Boost 1.80 in three days on 29th of June.

The rasterizers have not been released yet, so we are not breaking anything, IMO.

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.

Thanks!

@mloskot mloskot merged commit d5492e1 into boostorg:develop Jun 27, 2022
@mloskot mloskot added cat/feature New feature or functionality ext/rasterization boost/gil/extension/rasterization labels Jun 27, 2022
@mloskot mloskot added this to the Boost 1.80 milestone Jun 27, 2022
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)
Comment on lines -177 to +186
draw_curve(view, pixel, center, obtain_trajectory(semi_axes));
draw_curve(view, obtain_trajectory());
Copy link
Member

Choose a reason for hiding this comment

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

@marco-langer AFAICT, this draw_curve(view, obtain_trajectory()); invocation is not correct as the draw_curve still takes 3 arguments: view, pixel and trajectory_points.

The CI jobs for this PR were green though. It looks like this part is not covered by tests.
It will fail during compilation of example/rasterizer_ellipse.cpp though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix it and also add test cases

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/feature New feature or functionality ext/rasterization boost/gil/extension/rasterization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants