From d91b3e44a053d40d7e2fb899dbd8aee92905552f Mon Sep 17 00:00:00 2001 From: Henry Wong Date: Mon, 6 Oct 2025 00:44:07 -0700 Subject: [PATCH 1/2] Box3D: Fix applyAffine when applied to a subset of boxes of a Persp3D. If a transform is applied to some but not all 3dboxes of a perspective, the Persp3D is split, with a new Persp3D created for the transformed 3dboxes. In the current code, the new Persp3D isn't a copy of the existing one, causing weird behaviour (random translation and moved vanishing points). This commit fixes ObjectSet::applyAffine to actually copy the matrix (tmat) of the Box3D's Persp3D. Fixes https://gitlab.com/inkscape/inkscape/-/issues/3079 --- src/object/persp3d.cpp | 8 ++++++++ src/object/persp3d.h | 1 + src/selection-chemistry.cpp | 2 +- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/object/persp3d.cpp b/src/object/persp3d.cpp index d9f76f5e2e..219067a838 100644 --- a/src/object/persp3d.cpp +++ b/src/object/persp3d.cpp @@ -244,6 +244,14 @@ Persp3D::create_xml_element(SPDocument *document) { return reinterpret_cast( defs->get_child_by_repr(repr) ); } +Persp3D * +Persp3D::create_copy(const Persp3D ©_from) +{ + auto *p = create_xml_element(copy_from.document); + p->perspective_impl->tmat = copy_from.perspective_impl->tmat; + return p; +} + Persp3D * Persp3D::document_first_persp(SPDocument *document) { diff --git a/src/object/persp3d.h b/src/object/persp3d.h index dbba450dba..3eb7181ef4 100644 --- a/src/object/persp3d.h +++ b/src/object/persp3d.h @@ -109,6 +109,7 @@ public: static Persp3D * create_xml_element (SPDocument *document); static Persp3D * document_first_persp (SPDocument *document); + static Persp3D * create_copy(const Persp3D ©_from); bool has_all_boxes_in_selection (Inkscape::ObjectSet *set) const; diff --git a/src/selection-chemistry.cpp b/src/selection-chemistry.cpp index ace45449ae..104d0d7db7 100644 --- a/src/selection-chemistry.cpp +++ b/src/selection-chemistry.cpp @@ -1643,7 +1643,7 @@ void ObjectSet::applyAffine(Geom::Affine const &affine, bool set_i2d, bool compe if (persp) { if (!persp->has_all_boxes_in_selection (this)) { // create a new perspective as a copy of the current one - transf_persp = Persp3D::create_xml_element (persp->document); + transf_persp = Persp3D::create_copy(*persp); std::list selboxes = box3DList(persp); -- GitLab From 07cd077cdfcb6ec3da216f411f16763cd8be6c07 Mon Sep 17 00:00:00 2001 From: Henry Wong Date: Mon, 6 Oct 2025 03:45:45 -0700 Subject: [PATCH 2/2] Box3D: Similarly in vp_knot_moved_handler, fix Persp3D splitting Make a proper copy of the perspective when splitting a perspective between boxes. Fixes https://gitlab.com/inkscape/inkscape/-/issues/2874 Also replace a few std::list with std::vector/std::unordered_set for speed. --- src/object/persp3d.cpp | 15 +++------------ src/object/persp3d.h | 2 +- src/vanishing-point.cpp | 27 ++++++++++++--------------- src/vanishing-point.h | 3 ++- 4 files changed, 18 insertions(+), 29 deletions(-) diff --git a/src/object/persp3d.cpp b/src/object/persp3d.cpp index 219067a838..85b34e662e 100644 --- a/src/object/persp3d.cpp +++ b/src/object/persp3d.cpp @@ -471,18 +471,9 @@ Persp3D::update_z_orders () { } } -// FIXME: For some reason we seem to require a vector instead of a list in Persp3D, but in vp_knot_moved_handler() -// we need a list of boxes. If we can store a list in Persp3D right from the start, this function becomes -// obsolete. We should do this. -std::list +std::vector Persp3D::list_of_boxes() const { - auto persp_impl = perspective_impl.get(); - - std::list bx_lst; - for (auto & boxe : persp_impl->boxes) { - bx_lst.push_back(boxe); - } - return bx_lst; + return perspective_impl.get()->boxes; } bool @@ -498,7 +489,7 @@ Persp3D::absorb(Persp3D *other) { // Note: We first need to copy the boxes of other into a separate list; // otherwise the loop below gets confused when perspectives are reattached. - std::list boxes_of_persp2 = other->list_of_boxes(); + std::vector boxes_of_persp2 = other->list_of_boxes(); for (auto & box : boxes_of_persp2) { box->switch_perspectives(other, this, true); diff --git a/src/object/persp3d.h b/src/object/persp3d.h index 3eb7181ef4..98281745f9 100644 --- a/src/object/persp3d.h +++ b/src/object/persp3d.h @@ -102,7 +102,7 @@ public: void update_box_reprs (); void update_z_orders (); unsigned int num_boxes () const { return perspective_impl->boxes.size(); } - std::list list_of_boxes() const; + std::vector list_of_boxes() const; bool perspectives_coincide(Persp3D const *rhs) const; void absorb(Persp3D *persp2); diff --git a/src/vanishing-point.cpp b/src/vanishing-point.cpp index e3c3207721..0c11bc853a 100644 --- a/src/vanishing-point.cpp +++ b/src/vanishing-point.cpp @@ -90,23 +90,20 @@ static void vp_knot_moved_handler(SPKnot *knot, Geom::Point const &ppointer, gui if (dragger->numberOfBoxes() > 1) { // FIXME: Don't do anything if *all* boxes of a VP are selected std::set sel_vps = dragger->VPsOfSelectedBoxes(); - std::list sel_boxes; for (auto sel_vp : sel_vps) { // for each VP that has selected boxes: Persp3D *old_persp = sel_vp->get_perspective(); - sel_boxes = sel_vp->selectedBoxes(SP_ACTIVE_DESKTOP->getSelection()); + auto sel_boxes = sel_vp->selectedBoxes(SP_ACTIVE_DESKTOP->getSelection()); - // we create a new perspective ... - Persp3D *new_persp = Persp3D::create_xml_element(dragger->parent->document); + // create a copy of the existing perspective + Persp3D *new_persp = Persp3D::create_copy(*old_persp); - /* ... unlink the boxes from the old one and - FIXME: We need to unlink the _un_selected boxes of each VP so that - the correct boxes are kept with the VP being moved */ - std::list bx_lst = old_persp->list_of_boxes(); - for (auto & box : bx_lst) { - if (std::find(sel_boxes.begin(), sel_boxes.end(), box) == sel_boxes.end()) { + /* ... unlink unselected boxes of each VP so that + the selected boxes are kept with the VP being moved */ + for (auto *box : old_persp->list_of_boxes()) { + if (!sel_boxes.contains(box)) { /* if a box in the VP is unselected, move it to the - newly created perspective so that it doesn't get dragged **/ + newly created perspective so that it doesn't get dragged */ box->switch_perspectives(old_persp, new_persp); } } @@ -238,15 +235,15 @@ void VanishingPoint::set_pos(Proj::Pt2 const &pt) _persp->perspective_impl->tmat.set_image_pt(_axis, pt); } -std::list VanishingPoint::selectedBoxes(Inkscape::Selection *sel) +std::unordered_set VanishingPoint::selectedBoxes(Inkscape::Selection *sel) { - std::list sel_boxes; + std::unordered_set sel_boxes; auto itemlist = sel->items(); for (auto i = itemlist.begin(); i != itemlist.end(); ++i) { SPItem *item = *i; - auto box = cast(item); + auto *box = cast(item); if (box && this->hasBox(box)) { - sel_boxes.push_back(box); + sel_boxes.emplace(box); } } return sel_boxes; diff --git a/src/vanishing-point.h b/src/vanishing-point.h index 1b536ced2a..3185225123 100644 --- a/src/vanishing-point.h +++ b/src/vanishing-point.h @@ -16,6 +16,7 @@ #include <2geom/point.h> #include #include +#include #include "selection.h" @@ -88,7 +89,7 @@ public: } /* returns all selected boxes sharing this perspective */ - std::list selectedBoxes(Inkscape::Selection *sel); + std::unordered_set selectedBoxes(Inkscape::Selection *sel); inline void updateBoxDisplays() const { g_return_if_fail (_persp); -- GitLab