Add rasterizer support for ellipse

[meshtag]

Description

This pull request adds rasterization functionality for drawing ellipse using midpoint algorithm.

I would like to ponder upon following points once this basic functionality is approved(possibly merged) :

• Add support for drawing a rotated ellipse
• Vary thickness of curves drawn using rasterization
• Draw arcs(circular or elliptical) instead of drawing complete curves

I would like this pull request to be considered as solution for my competency test.

References

• An Efficient Ellipse-Drawing Algorithm, September 1984, pp. 24-35, vol. 4, by Jerry Van Aken, https://doi.ieeecomputersociety.org/10.1109/MCG.1984.275994

Tasklist

• Add test case(s)
• Ensure all CI builds pass
• Review and approve

[mloskot]

Please, ensure this code does not introduce any new warnings

[mloskot]

The feature is based on a known paper and is covered by a test.
So, without doing any verification of the algorithm implementation, I'm happy to accept it.

/cc @lpranam any objections?

[lpranam]

you forgot to add example in CMakeLists.txt otherwise all good :D

[meshtag]

As far as I understand, we don't need to add example files individually in example/CMakeLists.txt since it is using GLOB_RECURSE (unlike some other CMakeLists.txt files of the project).
Do you want me to change any other file? Please correct me if I am wrong.

[lpranam]

oh yes, sorry my bad I forgot. we have a few different approach in cmake so this happens a lot with me...

[marco-langer]

This PR was staged for releasing with Boost 1.80, but I'd like to give another review on ellipse.hpp:

• naming: why is it midpoint_circle_rasterizer, but midpoint_elliptical_rasterizer?
• the circle rasterizers take the center and radius arguments as signed integers whereas the ellipse rasterizer uses unsigned integers for semi-axes and center
• line and circle rasterizers implement the call operator to obtain the trajectory whereas the ellipse rasterizer implements the call operator for drawing the trajectory to an image
• the call operator makes a deep copy of the trajectory, because draw_curve() takes the vector by value
• why is the color implemented as a std::vector<unsigned int> and not as a pixel?
• why is error handling printed to std::cout?

[meshtag]

Thanks a lot for providing your review @marco-langer !

naming: why is it midpoint_circle_rasterizer, but midpoint_elliptical_rasterizer?

• naming : I agree that we can follow a more consistent naming pattern (maybe midpoint_ellipse_rasterizer instead of midpoint_elliptical_rasterizer ?)

the circle rasterizers take the center and radius arguments as signed integers whereas the ellipse rasterizer uses unsigned integers for semi-axes and center

• There is no significant point in taking semi-axes as signed integers. For center co-ordinates, I think we can chose to provide signed integer support here (though it might require significant changes in some other parts of the algorithm as well, for ex. checking of curve overflow in our frame). I can share more on this once I test/observe the usefulness of signed center co-ordinates in the case of midpoint_circle_rasterizer as well.

line and circle rasterizers implement the call operator to obtain the trajectory whereas the ellipse rasterizer implements the call operator for drawing the trajectory to an image

• I think my intention here was to save the user from the hassle of drawing stuff (with overflow checking).

the call operator makes a deep copy of the trajectory, because draw_curve() takes the vector by value

• Yes, I think we can probably try passing it by reference

why is the color implemented as a std::vector and not as a pixel?

• I don't really remember the reason I used std::vector<unsigned int> for color

why is error handling printed to std::cout?

• I don't remember the reason for using std::cout for indicating curve overflow, (perhaps I hesitated from using std::length_error because curve overflow is more like a warning/notification than an actual error). Please feel free to share your suggestions here as well.

[mloskot]

@marco-langer Great comments. Since this has already been merged, I'd suggest to simply open an issue with reference to this PR.
Unless @meshtag wants to attack your suggestions sooner and submit PR :)

[mloskot]

@meshtag FYI, we have merged PR #692 which addresses points raised by @marco-langer, so please have a look at those changes.

[meshtag]

Reviewed the PR.
@mloskot , thanks a lot for informing!