-
Notifications
You must be signed in to change notification settings - Fork 6
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
SimpleSets: basic implementation #46
Conversation
Note: for this to work in CLion, the working directory needs to be set to the project directory in the run configuration.
#include "function_painting.h" | ||
|
||
namespace cartocrow::renderer { | ||
FunctionPainting::FunctionPainting(const std::function<void(GeometryRenderer&)>& draw_function) |
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.
I think I don't grok the purpose of a FunctionPainting
. One can already make an ad-hoc painting by subclassing GeometryPainting
, for example:
class SomePainting : public GeometryPainting {
void paint(GeometryRenderer& renderer) const override {
renderer.draw(/* whatever you want to draw */);
}
};
IpeRenderer renderer(std::make_shared<SomePainting>());
I guess with FunctionPainting
slightly less code is needed:
IpeRenderer renderer(std::make_shared<FunctionPainting>([]() {
renderer.draw(/* whatever you want to draw */);
}));
but I'm not sure if the added complexity is worth it?
Anyway, did I miss a use case where using FunctionPainting
would be significantly easier?
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.
I would not use FunctionPainting on its own, but use the helper function (or the implicitly converting constructor that you suggest below).
Then you go from:
class SomePainting : public GeometryPainting {
void paint(GeometryRenderer& renderer) const override {
renderer.draw(/* whatever you want to draw */);
}
};
renderer.addPainting(std::make_shared<SomePainting>());
to
renderer.addPainting([](GeometryRenderer& renderer) {
renderer.draw(/* whatever you want to draw */);
});
I don't have a specific use case in mind that is way simpler this way, I added it only as a more concise way of drawing something simple. For me, having to create a class, derive from a specific other one, and override one specific method, in order to draw something is boiler plate and an obstacle when programming. Generally I would also define this painting class in a header, and have the implementation in the cpp file, though I see your point that you can also define it locally just in the cpp file. Part of the reason that I find this much simpler is probably that I'm more used to functional programming than object oriented programming.
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.
An argument for adding this: a lambda function makes it easy to capture variables that are in scope. When debugging a function, I sometimes use IpeRenderer together with a lambda function to draw some debug info and save it to a file.
@@ -680,6 +682,11 @@ void GeometryWidget::addPainting(std::shared_ptr<GeometryPainting> painting, con | |||
updateLayerList(); | |||
} | |||
|
|||
void GeometryWidget::addPainting(const std::function<void(renderer::GeometryRenderer&)>& draw_function, const std::string& name) { |
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.
(see comment about FunctionPainting
above) I'm also not a huge fan of having to do this in every renderer 🤔
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.
To avoid this while keeping FunctionPainting
we may provide an (implicitly converting) constructor from std::function<void(renderer::GeometryRenderer&)>
to FunctionPainting
?
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.
I did not think of that.
That does mean you cannot do
renderer.addPainting([](GeometryRenderer& renderer) {
renderer.draw(/* whatever you want to draw */);
});
because you need to still create a shared pointer, which I would like to avoid having to do.
We should at some point look into what helper functions can go into core. I have some helpers for arrangements that can be useful in general: connected components of faces in an arrangement (in particular circulators for going over the outer and inner boundaries of the component); extracting a general polygon from a circulator. |
Also fix spelling: admissable -> admissible
This PR implements the basis of SimpleSets.
Changes made to core and renderer:
nextPage
method to the ipe renderer, so that when called new added paintings are drawn on a new pageTODO:
Work around CGAL bug: Arrangement insert throws bad_get CGAL/cgal#8468Handle edges of dilated patterns that overlapFix code that draws shapes in a stacking order if possible, useful when wanting to import and/or modify the shapes later.