Rearranging Elements into groups
Closed this issue · 11 comments
I'm working on a function that groups together svg elements that area selected in an svg editor. To do this, I created a class called SelectionVisitor that contains a vector of selected elements called selectedElements.
I'm having a lot of problems with unique pointers though, since I have to store a reference to the selected Elements and then I have to move these Elements to a new group and delete them from the current Container. Here's the code to group the selected Elements in a new group (it's failing to compile):
int DomOperations::groupActiveSelection(std::string id) {
auto ge = utki::makeUnique<GElement>();
ge->id = id;
for (auto e : selectionVisitor.selectedElements) {
ge->children.push_back(std::move(e));
root.children.erase(e);
}
if (!ge->children.size() == 0)
root.children.push_back(std::move(ge));
selectionVisitor.selectedElements.clear();
return 0;
}And here's the code for the SelectionVisitor class:
class SelectionVisitor : virtual public svgdom::Visitor {
public:
std::vector<svgdom::Element*> selectedElements;
polygon_2f selectionPolygon;
void setSelectionPolygon(const polygon_2f selectionPolygon);
void visit(const svgdom::PathElement& e)override;
void visit(const svgdom::RectElement& e)override;
void visit(const svgdom::CircleElement& e)override;
void visit(const svgdom::EllipseElement& e)override;
void visit(const svgdom::LineElement& e)override;
void visit(const svgdom::PolylineElement& e)override;
void visit(const svgdom::PolygonElement& e)override;
void visit(const svgdom::GElement& e)override;
void visit(const svgdom::SvgElement& e)override;
void visit(const svgdom::SymbolElement& e)override;
void visit(const svgdom::UseElement& e)override;
void visit(const svgdom::DefsElement& e)override;
void visit(const svgdom::Gradient::StopElement& e)override;
void visit(const svgdom::LinearGradientElement& e)override;
void visit(const svgdom::RadialGradientElement& e)override;
void visit(const svgdom::FilterElement& e)override;
void visit(const svgdom::FeGaussianBlurElement& e)override;
};Do you have any suggestion on how to do this? I understand that a unique_ptr can't be copied, only moved, but this makes the problem of having references to selected Elements, and rearranging these elements into groups very difficult.
Hi Jaime,
to achieve that you need to store pairs of element's parent container and element pointer. I.e. something like this:
class SelectionVisitor : virtual public svgdom::Visitor {
svgdom::Container* curParent;
public:
std::vector<std::pair<svgdom::Container*, svgdom::Element*>> selectedElements;
void visit(const GElement& e)override{
if(e is selected){
add e and this->curParent to selectedElements;
return;
}
auto oldParent = this->curParent;
this->curParent = &e;
e.relayAccept(*this);
this->curParent = oldParent;
}
....
}
Then your group selection method will look like this:
int DomOperations::groupActiveSelection(std::string id) {
auto ge = utki::makeUnique<GElement>();
ge->id = id;
for (auto& e : selectionVisitor.selectedElements) {
//find child in parent
svgdom::Countainer* c = e.first;
for(auto i = c->children.begin(); i != c->children.end(); ++i){
//if element found
if(i->get() == e.second){
ge->children.push_back(std::move(*i));
c->children.erase(i);
break;
}
}
if (ge->children.size() != 0){
root.children.push_back(std::move(ge));
}
selectionVisitor.selectedElements.clear();
return 0;
}
I did not test the code, but it should give you some idea. Be also careful when removeing elements from some container, that removed element can be a parent to some other selected element which you will try to remove later and it can lead to some corruption, just think through if such situations are possible.
I think also that it mitgh be useful to change the type of Conatiner::children from vector to list, that would allow storing iterator i nstead of pointer to element object and then there will be no need to lookup for the element in the children vector. Ihave just implemented that.
Thanks for the help, I got it working, but I did had to make some changes. Here's the SelectionVisitor:
class SelectionVisitor : virtual public svgdom::Visitor {
const svgdom::Container* currentParent;
public:
std::vector<std::pair<const svgdom::Container*, const svgdom::Element*>> selectedElements;
void visitContainer(const svgdom::Element& e, const svgdom::Container& c) {
auto oldParent = this->currentParent;
this->currentParent = &c;
c.relayAccept(*this);
this->currentParent = oldParent;
}
....
void visit(const PathElement& e) override{
if (boost::geometry::within(e.absolutePolygon, selectionPolygon)) {
selectedPolygons.push_back(e.absolutePolygon);
selectedElements.push_back(std::make_pair(currentParent, (const Element*)&e));
}
}
....
}Notice that I had to change Container* and Element* to constants in selectedElements. These const will cause me problems later on the function to select and group:
int DomOperations::groupActiveSelection(string id) {
auto ge = utki::makeUnique<GElement>();
ge->id = Parser::getTokens(id);
for (auto e : selectionVisitor.selectedElements) {
Container* cont = const_cast<Container*>(e.first); // const to non-const cast is not recommended
for (auto i = cont->children.begin(); i != cont->children.end(); ++cont) {
if (i->get() == e.second) {
ge->children.push_back(std::move(*i));
cont->children.erase(i);
break;
}
}
}
if (ge->children.size() != 0) {
root.children.push_back(std::move(ge));
}
selectionVisitor.selectedElements.clear();
return 0;
}Notice that there is a cast from const to non-const to move and erase elements. This makes me really uncomfortable but I couldn't find a way around it, since the Visitor is forcing me to use constants.
I am also a bit uncomfortable with having to loop and compare all the Elements in the parent in order to remove selected elements.
I think also that it mitgh be useful to change the type of Conatiner::children from vector to list
Thanks! I actually made this exact change yesterday and was going to do the PR today, but you beat me to it.
By the way, quick note, in Github markdown if you use ```c++ instead of ``` for your code blocks you will get C++ highlights.
Ok, I understood your problem, and I need to think about how to solve it.
By the way, I think this cast:
(const Element*)&eis not necessary, if I'm not mistaken.
it looks like the solution would be to add non-const overloads of each visit() function in Visitor... doesn't look like a beautiful solution, but so far I don't see alternatives. I will think on it more, though.
yes, regarding (const Element*)&e cast, it is needed of course, but you should have used static_cast.
Avoid using C-style cast in C++ programs, it is a bad practice.
I will add a ConstVisitor which will have methods with const parameters and the Visitor will become non-const. Also, as a part of the change I will move Container::relayAccept() method to Visitor class.
Changes implemented. Please refer to the new test for usage idea:
https://github.com/igagis/svgdom/blob/master/tests/editDomVisitor/main.cpp
Thanks for the update!
I had to do some refactoring and I updated my custom visitor and it seems to be working now. The only little issue that I'm seeing is that Clion is failing to infer the type T_ChildIter since I'm using the member selectedElements from outside the visitor:
int DomOperations::setGroupAndTagsForSelection(string id) {
auto ge = utki::makeUnique<GElement>();
ge->id = id;
for(auto& p : selectionVisitor.selectedElements){
ge->children.push_back(std::move(*p.second));
p.first->children.erase(p.second);
}
if (ge->children.size() != 0) {
root.children.push_back(std::move(ge));
}
selectionVisitor.selectedElements.clear();
return 0;
}Clion shows the member selectedElements as vector<pair<Container*, <unknown>>>. But it compiles and runs correctly, so I'm guessing Clion is just having trouble inferring the correct type.
well, yes, it very well can be that CLion fails to infer the type. I use Netbeans and it has a lot of such failures, I don't even bother to complain about those :).