From b3a1fd967681c8f1120c4356c8bd87edf3326af9 Mon Sep 17 00:00:00 2001 From: wmayer Date: Mon, 23 Dec 2024 23:39:16 +0100 Subject: [PATCH 1/4] App: Add methods to get base and direction of datum element Add the methods DatumElement::getBasePoint() and DatumElement::getDirection() to hide implementation details and guarantee consistent behaviour. Using the methods fixes several regressions in: * Constraint::getDirection * PolarPattern::getTransformations --- src/App/Datums.cpp | 15 +++++++++++++++ src/App/Datums.h | 2 ++ src/Mod/Fem/App/FemConstraint.cpp | 6 ++---- src/Mod/PartDesign/App/FeatureLinearPattern.cpp | 4 +--- src/Mod/PartDesign/App/FeatureMirrored.cpp | 6 ++---- src/Mod/PartDesign/App/FeaturePolarPattern.cpp | 4 +--- src/Mod/PartDesign/App/FeatureSketchBased.cpp | 5 ++--- 7 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/App/Datums.cpp b/src/App/Datums.cpp index a551381c45..4f16c1ebff 100644 --- a/src/App/Datums.cpp +++ b/src/App/Datums.cpp @@ -87,6 +87,21 @@ bool DatumElement::isOriginFeature() return lcs ? lcs->isOrigin() : false; } +Base::Vector3d DatumElement::getBasePoint() const +{ + Base::Placement plc = Placement.getValue(); + return plc.getPosition(); +} + +Base::Vector3d DatumElement::getDirection() const +{ + Base::Vector3d dir(0.0, 0.0, 1.0); + Base::Placement plc = Placement.getValue(); + Base::Rotation rot = plc.getRotation(); + rot.multVec(dir, dir); + return dir; +} + // ---------------------------------------------------------------------------- LocalCoordinateSystem::LocalCoordinateSystem() diff --git a/src/App/Datums.h b/src/App/Datums.h index b199ffebd8..a99682f0c6 100644 --- a/src/App/Datums.h +++ b/src/App/Datums.h @@ -51,6 +51,8 @@ public: /// Finds the origin object this plane belongs to App::LocalCoordinateSystem* getLCS(); + Base::Vector3d getBasePoint() const; + Base::Vector3d getDirection() const; bool getCameraAlignmentDirection(Base::Vector3d& direction, const char* subname) const override; diff --git a/src/Mod/Fem/App/FemConstraint.cpp b/src/Mod/Fem/App/FemConstraint.cpp index e52c5b5ff3..91ae981391 100644 --- a/src/Mod/Fem/App/FemConstraint.cpp +++ b/src/Mod/Fem/App/FemConstraint.cpp @@ -516,14 +516,12 @@ const Base::Vector3d Constraint::getDirection(const App::PropertyLinkSub& direct } if (obj->isDerivedFrom()) { - Base::Vector3d vec(1.0, 0.0, 0.0); - static_cast(obj)->Placement.getValue().multVec(vec, vec); + Base::Vector3d vec = static_cast(obj)->getDirection(); return vec; } if (obj->isDerivedFrom()) { - Base::Vector3d vec(0.0, 0.0, 1.0); - static_cast(obj)->Placement.getValue().multVec(vec, vec); + Base::Vector3d vec = static_cast(obj)->getDirection(); return vec; } diff --git a/src/Mod/PartDesign/App/FeatureLinearPattern.cpp b/src/Mod/PartDesign/App/FeatureLinearPattern.cpp index 1a0265850f..58455b4624 100644 --- a/src/Mod/PartDesign/App/FeatureLinearPattern.cpp +++ b/src/Mod/PartDesign/App/FeatureLinearPattern.cpp @@ -161,9 +161,7 @@ const std::list LinearPattern::getTransformations(const std::vectorisDerivedFrom()) { App::Line* line = static_cast(refObject); - Base::Rotation rot = line->Placement.getValue().getRotation(); - Base::Vector3d d(1,0,0); - rot.multVec(d, d); + Base::Vector3d d = line->getDirection(); dir = gp_Dir(d.x, d.y, d.z); } else if (refObject->isDerivedFrom()) { if (subStrings[0].empty()) diff --git a/src/Mod/PartDesign/App/FeatureMirrored.cpp b/src/Mod/PartDesign/App/FeatureMirrored.cpp index 15af22543b..f9c530bf98 100644 --- a/src/Mod/PartDesign/App/FeatureMirrored.cpp +++ b/src/Mod/PartDesign/App/FeatureMirrored.cpp @@ -116,11 +116,9 @@ const std::list Mirrored::getTransformations(const std::vector(refObject)) { - Base::Vector3d base = plane->Placement.getValue().getPosition(); + Base::Vector3d base = plane->getBasePoint(); axbase = gp_Pnt(base.x, base.y, base.z); - Base::Rotation rot = plane->Placement.getValue().getRotation(); - Base::Vector3d dir(0,0,1); - rot.multVec(dir, dir); + Base::Vector3d dir = plane->getDirection(); axdir = gp_Dir(dir.x, dir.y, dir.z); return true; } diff --git a/src/Mod/PartDesign/App/FeaturePolarPattern.cpp b/src/Mod/PartDesign/App/FeaturePolarPattern.cpp index 9f337c571d..f2c4802f2b 100644 --- a/src/Mod/PartDesign/App/FeaturePolarPattern.cpp +++ b/src/Mod/PartDesign/App/FeaturePolarPattern.cpp @@ -129,9 +129,7 @@ const std::list PolarPattern::getTransformations(const std::vectorisDerivedFrom()) { App::Line* line = static_cast(refObject); - Base::Rotation rot = line->Placement.getValue().getRotation(); - Base::Vector3d d(1,0,0); - rot.multVec(d, d); + Base::Vector3d d = line->getDirection(); axdir = gp_Dir(d.x, d.y, d.z); } else if (refObject->isDerivedFrom()) { if (subStrings[0].empty()) diff --git a/src/Mod/PartDesign/App/FeatureSketchBased.cpp b/src/Mod/PartDesign/App/FeatureSketchBased.cpp index 92f22da99a..d04833e39b 100644 --- a/src/Mod/PartDesign/App/FeatureSketchBased.cpp +++ b/src/Mod/PartDesign/App/FeatureSketchBased.cpp @@ -1358,9 +1358,8 @@ void ProfileBased::getAxis(const App::DocumentObject * pcReferenceAxis, const st if (pcReferenceAxis->isDerivedFrom()) { auto* line = static_cast(pcReferenceAxis); - Base::Placement plc = line->Placement.getValue(); - base = plc.getPosition(); - plc.getRotation().multVec(Base::Vector3d(0, 0, 1), dir); + base = line->getBasePoint(); + dir = line->getDirection(); verifyAxisFunc(checkAxis, sketchplane, gp_Dir(dir.x, dir.y, dir.z)); return; From 90d8c34631707ba4e55ce1ba1973a8caeeb6b98b Mon Sep 17 00:00:00 2001 From: wmayer Date: Mon, 23 Dec 2024 23:55:00 +0100 Subject: [PATCH 2/4] App: Fix const correctness --- src/App/Datums.cpp | 6 +++--- src/App/Datums.h | 6 +++--- src/App/Origin.h | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/App/Datums.cpp b/src/App/Datums.cpp index 4f16c1ebff..2aa606a3bd 100644 --- a/src/App/Datums.cpp +++ b/src/App/Datums.cpp @@ -68,7 +68,7 @@ bool DatumElement::getCameraAlignmentDirection(Base::Vector3d& direction, const return true; } -App::LocalCoordinateSystem* DatumElement::getLCS() +App::LocalCoordinateSystem* DatumElement::getLCS() const { auto inList = getInList(); for (auto* obj : inList) { @@ -81,9 +81,9 @@ App::LocalCoordinateSystem* DatumElement::getLCS() return nullptr; } -bool DatumElement::isOriginFeature() +bool DatumElement::isOriginFeature() const { - auto lcs = getLCS(); + const auto* lcs = getLCS(); return lcs ? lcs->isOrigin() : false; } diff --git a/src/App/Datums.h b/src/App/Datums.h index a99682f0c6..a1a15663d9 100644 --- a/src/App/Datums.h +++ b/src/App/Datums.h @@ -50,14 +50,14 @@ public: ~DatumElement() override; /// Finds the origin object this plane belongs to - App::LocalCoordinateSystem* getLCS(); + App::LocalCoordinateSystem* getLCS() const; Base::Vector3d getBasePoint() const; Base::Vector3d getDirection() const; bool getCameraAlignmentDirection(Base::Vector3d& direction, const char* subname) const override; /// Returns true if this DatumElement is part of a App::Origin. - bool isOriginFeature(); + bool isOriginFeature() const; }; class AppExport Plane: public App::DatumElement @@ -199,7 +199,7 @@ public: /// Points types static constexpr const char* PointRoles[1] = {"Origin"}; - virtual bool isOrigin() + virtual bool isOrigin() const { return false; } diff --git a/src/App/Origin.h b/src/App/Origin.h index 9de0979fe7..268a4fcaf7 100644 --- a/src/App/Origin.h +++ b/src/App/Origin.h @@ -48,10 +48,10 @@ public: return "Gui::ViewProviderCoordinateSystem"; } - bool isOrigin() override + bool isOrigin() const override { return true; - }; + } }; } // namespace App From ad314fea7e29dd31ba3b190a8d9a76f5cd69ac8c Mon Sep 17 00:00:00 2001 From: wmayer Date: Sun, 29 Dec 2024 22:01:00 +0100 Subject: [PATCH 3/4] Core: Fixes #18717 Neither in #18332 nor in any of the referenced issues a reason is given why the placement of the coordinate axes must be changed. Only doing it to make the client code look more consistent is not a valid reason. This change breaks forward and backward compatibility and with #18717 we already have a hard to solve issue. Furthermore, at this point of time it's not even clear what other collateral damage the change may cause. So, this commit hides all the implementation details of the DatumElement and restores the original placement for the axes. This commit makes the line migration code obsolete --- src/App/Datums.cpp | 62 ++++++++++++++++---------------- src/App/Datums.h | 12 +++++-- src/Gui/ViewProviderLine.cpp | 14 +++++--- src/Mod/Part/App/PartFeature.cpp | 11 ++++-- src/Mod/Test/Document.py | 6 ++-- 5 files changed, 60 insertions(+), 45 deletions(-) diff --git a/src/App/Datums.cpp b/src/App/Datums.cpp index 2aa606a3bd..1d08929297 100644 --- a/src/App/Datums.cpp +++ b/src/App/Datums.cpp @@ -45,6 +45,7 @@ PROPERTY_SOURCE(App::Point, App::DatumElement) PROPERTY_SOURCE(App::LocalCoordinateSystem, App::GeoFeature) DatumElement::DatumElement(bool hideRole) + : baseDir{0.0, 0.0, 1.0} { ADD_PROPERTY_TYPE(Role, (""), @@ -63,7 +64,7 @@ DatumElement::~DatumElement() = default; bool DatumElement::getCameraAlignmentDirection(Base::Vector3d& direction, const char* subname) const { Q_UNUSED(subname); - Placement.getValue().getRotation().multVec(Base::Vector3d(0., 0., 1.), direction); + Placement.getValue().getRotation().multVec(baseDir, direction); return true; } @@ -95,13 +96,35 @@ Base::Vector3d DatumElement::getBasePoint() const Base::Vector3d DatumElement::getDirection() const { - Base::Vector3d dir(0.0, 0.0, 1.0); + Base::Vector3d dir(baseDir); Base::Placement plc = Placement.getValue(); Base::Rotation rot = plc.getRotation(); rot.multVec(dir, dir); return dir; } +Base::Vector3d DatumElement::getBaseDirection() const +{ + return baseDir; +} + +void DatumElement::setBaseDirection(const Base::Vector3d& dir) +{ + baseDir = dir; +} + +// ---------------------------------------------------------------------------- + +Line::Line() +{ + setBaseDirection(Base::Vector3d(1, 0, 0)); +} + +Point::Point() +{ + setBaseDirection(Base::Vector3d(0, 0, 0)); +} + // ---------------------------------------------------------------------------- LocalCoordinateSystem::LocalCoordinateSystem() @@ -221,9 +244,9 @@ const std::vector& LocalCoordinateSystem::getS { static const std::vector setupData = { // clang-format off - {App::Line::getClassTypeId(), AxisRoles[0], tr("X-axis"), Base::Rotation(Base::Vector3d(1, 1, 1), M_PI * 2 / 3)}, - {App::Line::getClassTypeId(), AxisRoles[1], tr("Y-axis"), Base::Rotation(Base::Vector3d(-1, 1, 1), M_PI * 2 / 3)}, - {App::Line::getClassTypeId(), AxisRoles[2], tr("Z-axis"), Base::Rotation()}, + {App::Line::getClassTypeId(), AxisRoles[0], tr("X-axis"), Base::Rotation()}, + {App::Line::getClassTypeId(), AxisRoles[1], tr("Y-axis"), Base::Rotation(Base::Vector3d(1, 1, 1), M_PI * 2 / 3)}, + {App::Line::getClassTypeId(), AxisRoles[2], tr("Z-axis"), Base::Rotation(Base::Vector3d(1,-1, 1), M_PI * 2 / 3)}, {App::Plane::getClassTypeId(), PlaneRoles[0], tr("XY-plane"), Base::Rotation()}, {App::Plane::getClassTypeId(), PlaneRoles[1], tr("XZ-plane"), Base::Rotation(1.0, 0.0, 0.0, 1.0)}, {App::Plane::getClassTypeId(), PlaneRoles[2], tr("YZ-plane"), Base::Rotation(Base::Vector3d(1, 1, 1), M_PI * 2 / 3)}, @@ -233,7 +256,7 @@ const std::vector& LocalCoordinateSystem::getS return setupData; } -DatumElement* LocalCoordinateSystem::createDatum(SetupData& data) +DatumElement* LocalCoordinateSystem::createDatum(const SetupData& data) { App::Document* doc = getDocument(); std::string objName = doc->getUniqueObjectName(data.role); @@ -297,10 +320,6 @@ void LocalCoordinateSystem::onDocumentRestored() // In 0.22 origins did not have point. migrateOriginPoint(); - - // In 0.22 the axis placement were wrong. The X axis had identity placement instead of the Z. - // This was fixed but we need to migrate old files. - migrateXAxisPlacement(); } void LocalCoordinateSystem::migrateOriginPoint() @@ -314,33 +333,12 @@ void LocalCoordinateSystem::migrateOriginPoint() if (std::none_of(features.begin(), features.end(), isOrigin)) { auto data = getData(PointRoles[0]); auto* origin = createDatum(data); + origin->purgeTouched(); features.push_back(origin); OriginFeatures.setValues(features); } } -void LocalCoordinateSystem::migrateXAxisPlacement() -{ - constexpr const double tolerance = 1e-5; - auto features = OriginFeatures.getValues(); - - const auto& setupData = getSetupData(); - for (auto* obj : features) { - auto* feature = dynamic_cast (obj); - if (!feature) { continue; } - for (auto data : setupData) { - // ensure the rotation is correct for the role - if (std::strcmp(feature->Role.getValue(), data.role) == 0) { - if (!feature->Placement.getValue().getRotation().isSame(data.rot, tolerance)) { - feature->Placement.setValue(Base::Placement(Base::Vector3d(), data.rot)); - getDocument()->setStatus(App::Document::MigrateLCS, true); - } - break; - } - } - } -} - // ---------------------------------------------------------------------------- LocalCoordinateSystem::LCSExtension::LCSExtension(LocalCoordinateSystem* obj) diff --git a/src/App/Datums.h b/src/App/Datums.h index a1a15663d9..9426f4afc5 100644 --- a/src/App/Datums.h +++ b/src/App/Datums.h @@ -53,11 +53,18 @@ public: App::LocalCoordinateSystem* getLCS() const; Base::Vector3d getBasePoint() const; Base::Vector3d getDirection() const; + Base::Vector3d getBaseDirection() const; bool getCameraAlignmentDirection(Base::Vector3d& direction, const char* subname) const override; /// Returns true if this DatumElement is part of a App::Origin. bool isOriginFeature() const; + +protected: + void setBaseDirection(const Base::Vector3d& dir); + +private: + Base::Vector3d baseDir; }; class AppExport Plane: public App::DatumElement @@ -76,6 +83,7 @@ class AppExport Line: public App::DatumElement PROPERTY_HEADER_WITH_OVERRIDE(App::DatumElement); public: + Line(); const char* getViewProviderName() const override { return "Gui::ViewProviderLine"; @@ -87,6 +95,7 @@ class AppExport Point: public App::DatumElement PROPERTY_HEADER_WITH_OVERRIDE(App::DatumElement); public: + Point(); const char* getViewProviderName() const override { return "Gui::ViewProviderPoint"; @@ -245,11 +254,10 @@ private: }; static const std::vector& getSetupData(); - DatumElement* createDatum(SetupData& data); + DatumElement* createDatum(const SetupData& data); SetupData getData(const char* role); void migrateOriginPoint(); - void migrateXAxisPlacement(); }; } // namespace App diff --git a/src/Gui/ViewProviderLine.cpp b/src/Gui/ViewProviderLine.cpp index d2e2482c32..33fc82721b 100644 --- a/src/Gui/ViewProviderLine.cpp +++ b/src/Gui/ViewProviderLine.cpp @@ -33,6 +33,7 @@ #endif #include +#include #include #include "ViewProviderLine.h" @@ -53,7 +54,8 @@ ViewProviderLine::ViewProviderLine() ViewProviderLine::~ViewProviderLine() = default; -void ViewProviderLine::attach(App::DocumentObject *obj) { +void ViewProviderLine::attach(App::DocumentObject *obj) +{ ViewProviderDatum::attach(obj); // Setup label text and line colors @@ -82,14 +84,16 @@ void ViewProviderLine::attach(App::DocumentObject *obj) { static const float size = App::GetApplication().GetParameterGroupByPath("User parameter:BaseApp/Preferences/View")->GetFloat("DatumLineSize", 70.0); + auto line = getObject(); + Base::Vector3d dir = line->getBaseDirection(); SbVec3f verts[2]; if (noRole) { - verts[0] = SbVec3f(0, 0, 2 * size); + verts[0] = Base::convertTo(dir * 2 * size); verts[1] = SbVec3f(0, 0, 0); } else { - verts[0] = SbVec3f(0, 0, size); - verts[1] = SbVec3f(0, 0, 0.2 * size); + verts[0] = Base::convertTo(dir * size); + verts[1] = Base::convertTo(dir * 0.2 * size); } // indexes used to create the edges @@ -108,7 +112,7 @@ void ViewProviderLine::attach(App::DocumentObject *obj) { sep->addChild ( pLines ); auto textTranslation = new SoTranslation (); - textTranslation->translation.setValue(SbVec3f(0, 0, size * 1.1)); + textTranslation->translation.setValue(Base::convertTo(dir * 1.1 * size)); sep->addChild ( textTranslation ); auto ps = new SoPickStyle(); diff --git a/src/Mod/Part/App/PartFeature.cpp b/src/Mod/Part/App/PartFeature.cpp index b4550f49a3..2f007b9e2b 100644 --- a/src/Mod/Part/App/PartFeature.cpp +++ b/src/Mod/Part/App/PartFeature.cpp @@ -69,6 +69,7 @@ #include #include #include +#include #include #include "Geometry.h" @@ -76,7 +77,7 @@ #include "PartFeaturePy.h" #include "PartPyCXX.h" #include "TopoShapePy.h" -#include "Base/Tools.h" +#include "Tools.h" using namespace Part; namespace sp = std::placeholders; @@ -1026,7 +1027,9 @@ static TopoShape _getTopoShape(const App::DocumentObject* obj, if (linked->isDerivedFrom(App::Line::getClassTypeId())) { static TopoDS_Shape _shape; if (_shape.IsNull()) { - BRepBuilderAPI_MakeEdge builder(gp_Lin(gp_Pnt(0, 0, 0), gp_Dir(0, 0, 1))); + auto line = static_cast(linked); + Base::Vector3d dir = line->getBaseDirection(); + BRepBuilderAPI_MakeEdge builder(gp_Lin(gp_Pnt(0, 0, 0), Base::convertTo(dir))); _shape = builder.Shape(); _shape.Infinite(Standard_True); } @@ -1035,7 +1038,9 @@ static TopoShape _getTopoShape(const App::DocumentObject* obj, else if (linked->isDerivedFrom(App::Plane::getClassTypeId())) { static TopoDS_Shape _shape; if (_shape.IsNull()) { - BRepBuilderAPI_MakeFace builder(gp_Pln(gp_Pnt(0, 0, 0), gp_Dir(0, 0, 1))); + auto plane = static_cast(linked); + Base::Vector3d dir = plane->getBaseDirection(); + BRepBuilderAPI_MakeFace builder(gp_Pln(gp_Pnt(0, 0, 0), Base::convertTo(dir))); _shape = builder.Shape(); _shape.Infinite(Standard_True); } diff --git a/src/Mod/Test/Document.py b/src/Mod/Test/Document.py index 4959650789..4a08d51c4b 100644 --- a/src/Mod/Test/Document.py +++ b/src/Mod/Test/Document.py @@ -357,17 +357,17 @@ class DocumentBasicCases(unittest.TestCase): res = obj.getSubObject("X_Axis", retType=2) self.assertEqual( - res[1].multVec(FreeCAD.Vector(0, 0, 1)).getAngle(FreeCAD.Vector(1, 0, 0)), 0.0 + res[1].multVec(FreeCAD.Vector(1, 0, 0)).getAngle(FreeCAD.Vector(1, 0, 0)), 0.0 ) res = obj.getSubObject("Y_Axis", retType=2) self.assertEqual( - res[1].multVec(FreeCAD.Vector(0, 0, 1)).getAngle(FreeCAD.Vector(0, 1, 0)), 0.0 + res[1].multVec(FreeCAD.Vector(1, 0, 0)).getAngle(FreeCAD.Vector(0, 1, 0)), 0.0 ) res = obj.getSubObject("Z_Axis", retType=2) self.assertEqual( - res[1].multVec(FreeCAD.Vector(0, 0, 1)).getAngle(FreeCAD.Vector(0, 0, 1)), 0.0 + res[1].multVec(FreeCAD.Vector(1, 0, 0)).getAngle(FreeCAD.Vector(0, 0, 1)), 0.0 ) res = obj.getSubObject("XY_Plane", retType=2) From 7cfa0df4ade8b613359aebcb92e6190cae81e4bf Mon Sep 17 00:00:00 2001 From: wmayer Date: Sun, 29 Dec 2024 22:03:37 +0100 Subject: [PATCH 4/4] Gui: Remove migration warning --- src/Gui/CommandDoc.cpp | 31 ------------------------------- 1 file changed, 31 deletions(-) diff --git a/src/Gui/CommandDoc.cpp b/src/Gui/CommandDoc.cpp index f6791360ad..86e7fdf0a4 100644 --- a/src/Gui/CommandDoc.cpp +++ b/src/Gui/CommandDoc.cpp @@ -114,36 +114,6 @@ void StdCmdOpen::activated(int iMsg) "likely result in loss of data.")); } }; - - auto checkMigrationLCS = [](App::Document* doc) { - if (doc && doc->testStatus(App::Document::MigrateLCS)) { - auto grp = App::GetApplication().GetParameterGroupByPath("User parameter:BaseApp/Preferences/View"); - if (!grp->GetBool("ShowLCSMigrationWarning", true)) { - return; - } - - // Display the warning message - QMessageBox msgBox(QMessageBox::Warning, - QObject::tr("File Migration Warning"), - QObject::tr("This file was created with an older version of %1. " - "Origin axes had incorrect placements, which have now been corrected.\n\n" - "However, if you save this file in the current version and reopen it in an" - " older version of %1, the origin axes will be misaligned. Additionally, " - "if your file references these origin axes, your file will likely be broken.") - .arg(QApplication::applicationName()), - QMessageBox::Ok); - - QCheckBox* checkBox = new QCheckBox(QObject::tr("Don't show this warning again")); - msgBox.setCheckBox(checkBox); - - msgBox.exec(); - - // Save preference if the user selects "Don't show again" - if (checkBox->isChecked()) { - grp->SetBool("ShowLCSMigrationWarning", false); - } - } - }; // clang-format on Q_UNUSED(iMsg); @@ -215,7 +185,6 @@ void StdCmdOpen::activated(int iMsg) checkPartialRestore(doc); checkRestoreError(doc); - checkMigrationLCS(doc); } } }