Skip to content
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

Add spacenavd motion event support to Placement dialog that allows for object rotation and translation #18098

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions src/Gui/Placement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,30 +47,35 @@
#include <Gui/Window.h>

#include "Placement.h"
#include "ui_Placement.h"

Check failure on line 50 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

'ui_Placement.h' file not found [clang-diagnostic-error]

#include <Gui/MainWindow.h>
#include <Gui/View3DInventor.h>
#include <Inventor/SbVec3f.h>
#include <Gui/SpaceballEvent.h>
#include <Base/DualQuaternion.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header seems to be superfluous

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, every thing except
#include <Gui/SpaceballEvent.h>
seems superfluous and are left from old experiments.


using namespace Gui::Dialog;
namespace sp = std::placeholders;

namespace Gui { namespace Dialog {

Check warning on line 61 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

nested namespaces can be concatenated [modernize-concat-nested-namespaces]
class find_placement
{
public:
explicit find_placement(const std::string& name) : propertyname(name)

Check warning on line 65 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

pass by value and use std::move [modernize-pass-by-value]
{
}
bool operator () (const std::pair<std::string, App::Property*>& elem) const
{
if (elem.first == propertyname) {
// flag set that property is read-only or hidden
if (elem.second->testStatus(App::Property::ReadOnly) || elem.second->testStatus(App::Property::Hidden))

Check warning on line 72 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

statement should be inside braces [readability-braces-around-statements]
return false;
App::PropertyContainer* parent = elem.second->getContainer();
if (parent) {
// flag set that property is read-only or hidden
if (parent->isReadOnly(elem.second) ||
parent->isHidden(elem.second))

Check warning on line 78 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

statement should be inside braces [readability-braces-around-statements]
return false;
}
return elem.second->isDerivedFrom
Expand All @@ -83,11 +88,11 @@
static App::PropertyPlacement* getProperty(const App::DocumentObject* obj,
const std::string& propertyName)
{
std::map<std::string,App::Property*> props;

Check warning on line 91 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable 'props' is not initialized [cppcoreguidelines-init-variables]
obj->getPropertyMap(props);

// search for the placement property
std::map<std::string,App::Property*>::iterator jt;

Check warning on line 95 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable 'jt' is not initialized [cppcoreguidelines-init-variables]
jt = std::find_if(props.begin(), props.end(), find_placement(propertyName));
if (jt != props.end()) {
return dynamic_cast<App::PropertyPlacement*>(jt->second);
Expand Down Expand Up @@ -179,7 +184,7 @@
void PlacementHandler::openTransaction()
{
App::Document* activeDoc = App::GetApplication().getActiveDocument();
if (activeDoc)

Check warning on line 187 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

statement should be inside braces [readability-braces-around-statements]
activeDoc->openTransaction("Placement");
}

Expand All @@ -201,7 +206,7 @@
std::vector<const App::DocumentObject*> PlacementHandler::getObjects(const Gui::Document* document) const
{
auto objs = document->getDocument()->getObjectsOfType(App::DocumentObject::getClassTypeId());
std::vector<const App::DocumentObject*> list;

Check warning on line 209 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable 'list' is not initialized [cppcoreguidelines-init-variables]
list.insert(list.begin(), objs.begin(), objs.end());
return list;
}
Expand All @@ -209,7 +214,7 @@
std::vector<const App::DocumentObject*> PlacementHandler::getSelectedObjects(const Gui::Document* document) const
{
App::Document* doc = document->getDocument();
std::vector<const App::DocumentObject*> list;

Check warning on line 217 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable 'list' is not initialized [cppcoreguidelines-init-variables]
list.reserve(selectionObjects.size());
for (const auto& it : selectionObjects) {
const App::DocumentObject* obj = it.getObject();
Expand All @@ -228,7 +233,7 @@

void PlacementHandler::revertTransformationOfViewProviders(Gui::Document* document)
{
std::vector<const App::DocumentObject*> obj = getObjects(document);

Check warning on line 236 in src/Gui/Placement.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint

variable 'obj' is not initialized [cppcoreguidelines-init-variables]
for (const auto & it : obj) {
auto property = find_placement::getProperty(it, this->propertyName);
if (property) {
Expand Down Expand Up @@ -462,10 +467,12 @@
setupUnits();
setupSignalMapper();
setupRotationMethod();
setupEventFilter();
}

Placement::~Placement()
{
if (qApp!=NULL) qApp->removeEventFilter(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is not needed. I have never seen qApp being null in a GUI application. Btw, NULL is C and in modern C++ nullptr must be used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check removed.

Assumed that it mitigated a segmentation fault that I have seen.
Unfortunately triggering it was not very repeatable and this check does not help with that segfault.
I can also trigger that segfault on current unmodified Freecad:main branch.

Could you check if you can trigger it by something like this:

  1. open any document (lets say "test.FCStd")
  2. open placement dialog on any Body in that document
  3. move object by spacenavd or mouse scroll wheel on angle spinbox
  4. open any other document without closing placement dialog
  5. close test.FCStd
  6. move object by spacenavd or mouse scroll wheel on angle spinbox (trigger is hard to predict, sometimes value needs to be changed many times per second or enough times to trigger, sometimes a single value change can trigger a segmentation fault, I have seen a higher probability with spacenavd)

After that procedure i can get different segmentation faults.
For example like this:

No object selected. Program received signal SIGSEGV, Segmentation fault. #0 /lib/x86_64-linux-gnu/libc.so.6(+0x3c050) [0x7fdd3e369050] #1 0x7fdd3e67e014 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::c_str() const from /lib/x86_64-linux-gnu/libstdc++.so.6+0x4 #2 0x7fdd4139019c in App::Document::getName() const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x1e #3 0x7fdd41605a1e in App::ObjectIdentifier::resolve(App::ObjectIdentifier::ResolveResults&) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x114 #4 0x7fdd4160c60c in App::ObjectIdentifier::ResolveResults::ResolveResults(App::ObjectIdentifier const&) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x1ec #5 0x7fdd41608105 in App::ObjectIdentifier::getDocumentName() const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x33 #6 0x7fdd416066cc in App::ObjectIdentifier::getDocument(App::ObjectIdentifier::String, bool*) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x52 #7 0x7fdd41606970 in App::ObjectIdentifier::getDocumentObject() const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x6e #8 0x7fdd4359baba in Gui::QuantitySpinBoxPrivate::parseString(QString const&, Base::Quantity&, double&, App::ObjectIdentifier const&) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0xbe #9 0x7fdd4359ccdc in Gui::QuantitySpinBoxPrivate::validateAndInterpret(QString&, QValidator::State&, App::ObjectIdentifier const&) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x1012 #10 0x7fdd4359a27b in Gui::QuantitySpinBox::validate(QString&, int&) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x57 #11 0x7fdd3f660868 in QWidgetLineControl::finishChange(int, bool, bool) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0xf8 #12 0x7fdd3f660c2e in QWidgetLineControl::internalSetText(QString const&, int, bool) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1ae #13 0x7fdd43598031 in Gui::QuantitySpinBox::updateEdit(QString const&) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0xaf #14 0x7fdd43597f43 in Gui::QuantitySpinBox::updateText(Base::Quantity const&) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x7f #15 0x7fdd43599271 in Gui::QuantitySpinBox::stepBy(int) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0xdf #16 0x7fdd3f6b46e1 in QAbstractSpinBox::wheelEvent(QWheelEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x81 #17 0x7fdd3f551db8 in QWidget::event(QEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1a8 #18 0x7fdd4359983d in Gui::QuantitySpinBox::event(QEvent*) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x23 #19 0x7fdd3f50ffae in QApplicationPrivate::notify_helper(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x7e #20 0x7fdd3f518e06 in QApplication::notify(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1c76 #21 0x7fdd42eef000 in Gui::GUIApplication::notify(QObject*, QEvent*) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x134 #22 0x7fdd3ea3b738 in QCoreApplication::notifyInternal2(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x118 #23 /lib/x86_64-linux-gnu/libQt5Widgets.so.5(+0x1bf7ab) [0x7fdd3f56c7ab] #24 /lib/x86_64-linux-gnu/libQt5Widgets.so.5(+0x1c1095) [0x7fdd3f56e095] #25 0x7fdd3f50ffae in QApplicationPrivate::notify_helper(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x7e #26 0x7fdd42eef000 in Gui::GUIApplication::notify(QObject*, QEvent*) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x134 #27 0x7fdd3ea3b738 in QCoreApplication::notifyInternal2(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x118 #28 0x7fdd3ee16e72 in QGuiApplicationPrivate::processWheelEvent(QWindowSystemInterfacePrivate::WheelEvent*) from /lib/x86_64-linux-gnu/libQt5Gui.so.5+0xe2 #29 0x7fdd3edefcec in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) from /lib/x86_64-linux-gnu/libQt5Gui.so.5+0xac #30 /lib/x86_64-linux-gnu/libQt5XcbQpa.so.5(+0x6deca) [0x7fdd38327eca] #31 /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_dispatch+0x299) [0x7fdd3c8767a9] #32 /lib/x86_64-linux-gnu/libglib-2.0.so.0(+0x54a38) [0x7fdd3c876a38] #33 /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_iteration+0x2c) [0x7fdd3c876acc] #34 0x7fdd3ea93876 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x66 #35 0x7fdd3ea3a1bb in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x12b #36 0x7fdd3ea42316 in QCoreApplication::exec() from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x96 #37 /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so(+0x129ad91) [0x7fdd42dabd91] #38 /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so(+0x129b055) [0x7fdd42dac055] #39 0x7fdd42dac323 in Gui::Application::runApplication() from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x1d3 #40 ./bin/FreeCAD(+0x2d683) [0x559ae0063683] #41 /lib/x86_64-linux-gnu/libc.so.6(+0x2724a) [0x7fdd3e35424a] #42 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x85) [0x7fdd3e354305] #43 ./bin/FreeCAD(+0x2c801) [0x559ae0062801]

or

Program received signal SIGSEGV, Segmentation fault. #0 /lib/x86_64-linux-gnu/libc.so.6(+0x3c050) [0x7f67c8eee050] #1 0x7f67c9203014 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::c_str() const from /lib/x86_64-linux-gnu/libstdc++.so.6+0x4 #2 0x7f67cbf1519c in App::Document::getName() const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x1e #3 0x7f67cc18aa1e in App::ObjectIdentifier::resolve(App::ObjectIdentifier::ResolveResults&) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x114 #4 0x7f67cc19160c in App::ObjectIdentifier::ResolveResults::ResolveResults(App::ObjectIdentifier const&) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x1ec #5 0x7f67cc18d105 in App::ObjectIdentifier::getDocumentName() const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x33 #6 0x7f67cc18b6cc in App::ObjectIdentifier::getDocument(App::ObjectIdentifier::String, bool*) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x52 #7 0x7f67cc18b970 in App::ObjectIdentifier::getDocumentObject() const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADApp.so+0x6e #8 0x7f67ce120aba in Gui::QuantitySpinBoxPrivate::parseString(QString const&, Base::Quantity&, double&, App::ObjectIdentifier const&) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0xbe #9 0x7f67ce121cdc in Gui::QuantitySpinBoxPrivate::validateAndInterpret(QString&, QValidator::State&, App::ObjectIdentifier const&) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x1012 #10 0x7f67ce11f27b in Gui::QuantitySpinBox::validate(QString&, int&) const from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x57 #11 0x7f67ca1e5868 in QWidgetLineControl::finishChange(int, bool, bool) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0xf8 #12 0x7f67ca1e5c2e in QWidgetLineControl::internalSetText(QString const&, int, bool) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1ae #13 0x7f67ce11d031 in Gui::QuantitySpinBox::updateEdit(QString const&) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0xaf #14 0x7f67ce11cf43 in Gui::QuantitySpinBox::updateText(Base::Quantity const&) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x7f #15 0x7f67ce11e271 in Gui::QuantitySpinBox::stepBy(int) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0xdf #16 0x7f67ca2396e1 in QAbstractSpinBox::wheelEvent(QWheelEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x81 #17 0x7f67ca0d6db8 in QWidget::event(QEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1a8 #18 0x7f67ce11e83d in Gui::QuantitySpinBox::event(QEvent*) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x23 #19 0x7f67ca094fae in QApplicationPrivate::notify_helper(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x7e #20 0x7f67ca09de06 in QApplication::notify(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1c76 #21 0x7f67cda74000 in Gui::GUIApplication::notify(QObject*, QEvent*) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x134 #22 0x7f67c95c0738 in QCoreApplication::notifyInternal2(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x118 #23 /lib/x86_64-linux-gnu/libQt5Widgets.so.5(+0x1bf7ab) [0x7f67ca0f17ab] #24 /lib/x86_64-linux-gnu/libQt5Widgets.so.5(+0x1c1095) [0x7f67ca0f3095] #25 0x7f67ca094fae in QApplicationPrivate::notify_helper(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x7e #26 0x7f67cda74000 in Gui::GUIApplication::notify(QObject*, QEvent*) from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x134 #27 0x7f67c95c0738 in QCoreApplication::notifyInternal2(QObject*, QEvent*) from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x118 #28 0x7f67c999be72 in QGuiApplicationPrivate::processWheelEvent(QWindowSystemInterfacePrivate::WheelEvent*) from /lib/x86_64-linux-gnu/libQt5Gui.so.5+0xe2 #29 0x7f67c9974cec in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) from /lib/x86_64-linux-gnu/libQt5Gui.so.5+0xac #30 /lib/x86_64-linux-gnu/libQt5XcbQpa.so.5(+0x6deca) [0x7f67c2eaceca] #31 /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_dispatch+0x299) [0x7f67c73fb7a9] #32 /lib/x86_64-linux-gnu/libglib-2.0.so.0(+0x54a38) [0x7f67c73fba38] #33 /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_iteration+0x2c) [0x7f67c73fbacc] #34 0x7f67c9618876 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x66 #35 0x7f67c95bf1bb in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x12b #36 0x7f67c95c7316 in QCoreApplication::exec() from /lib/x86_64-linux-gnu/libQt5Core.so.5+0x96 #37 /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so(+0x129ad91) [0x7f67cd930d91] #38 /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so(+0x129b055) [0x7f67cd931055] #39 0x7f67cd931323 in Gui::Application::runApplication() from /freecad_build/github2/FreeCAD/_build/lib/libFreeCADGui.so+0x1d3 #40 ./bin/FreeCAD(+0x2d683) [0x5601139e0683] #41 /lib/x86_64-linux-gnu/libc.so.6(+0x2724a) [0x7f67c8ed924a] #42 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x85) [0x7f67c8ed9305] #43 ./bin/FreeCAD(+0x2c801) [0x5601139df801]

Copy link
Contributor

@wwmayer wwmayer Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I don't have a space mouse device.

From the callstack it looks like a null string is assigned to a std::string. This is not allowed and may cause a segmentation fault or throws an exception.

delete ui;
}

Expand Down Expand Up @@ -547,6 +554,12 @@
ui->stackedWidget->setCurrentIndex(index);
}


void Placement::setupEventFilter()
{
if (qApp!=NULL) qApp->installEventFilter(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

}

void Placement::showDefaultButtons(bool ok)
{
ui->buttonBox->setVisible(ok);
Expand All @@ -559,6 +572,83 @@
}
}

bool Placement::eventFilter(QObject*, QEvent* ev) {
//handle spacenav daemon motion events in placement window
if (//event->type() == Spaceball::ButtonEvent::ButtonEventType ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented code should be removed.

ev->type() == Spaceball::MotionEvent::MotionEventType){
Spaceball::MotionEvent *motionEvent = dynamic_cast<Spaceball::MotionEvent*>(ev);
if (!motionEvent) {
Base::Console().Log("invalid spaceball motion event in bool Placement::eventFilter(QObject*, QEvent* ev)\n");
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation error.

motionEvent->setHandled(true);

static double translationConstant(.001f);
double xTrans = static_cast<double>(motionEvent->translationX())*translationConstant;
double yTrans = static_cast<double>(motionEvent->translationY())*translationConstant;
double zTrans = static_cast<double>(motionEvent->translationZ())*translationConstant;

static double rotationConstant(.001f);
double dY=static_cast<double>(motionEvent->rotationZ()) * rotationConstant;
double dP=static_cast<double>(motionEvent->rotationY()) * rotationConstant;
double dR=static_cast<double>(motionEvent->rotationX()) * rotationConstant;

if(xTrans == 0 && yTrans == 0 && zTrans == 0 && dY ==0 && dP ==0 && dR ==0 ) return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To improve readability you should write:

if (xTrans == 0 && yTrans == 0 && zTrans == 0 && dY ==0 && dP ==0 && dR ==0) {
    return false;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or simply

return  xTrans != 0 || yTrans != 0 || zTrans != 0 || dY !=0 || dP !=0 || dR !=0;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bofdahof doing that would return from the function too early, as there are more lines after this if.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! My bad!!! You are quite correct!

signalMapper->blockSignals(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend to use: QSignalBlocker blocker(signalMapper); because this is exception-safe and you don't have to worry about unblocking it when leaving the method (RAII).

// printf("\nSpaceball Placement Event\t(x,y,z)=(%f,%f,%f)\t(Rx,Ry,Rz)=(%f,%f,%f);\n",xTrans,yTrans,zTrans,dR,dP,dY );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed commented code

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am developing a 6dof controller and this snippet is useful for debugging purposes.
I am not very familiar how debugging code is usually included in Freecad.
Could you recommend an acceptable way to include it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer, I don't know what's preferred in freecad. This is just my personal opinion.

The issue I have with including debug code in a non-personal project, is that the debug code is essentially dead code for everyone except the author. Debug code usually make code more difficult to read and makes it more difficult to maintain and refactor.

When coding, I try to prioritize others time above my own. I'm only one person, but there might be hundreds of people who my laziness affects.

Personally, I do the following:

  1. don't stage debug code when creating commits
  2. stash and test without debug code before push
  3. If I want to keep the debug code, I create a new branch and a a commit the debug code and call it DEBUG, DONT PUSH to avoid accidentally create a pull request with that code.

Later when/if I want to continue with the debugging, I can simply checkout and rebase the debug code on latest main. There might be conflicts this way, but that just shows that it was a good idea to avoid commit that code in the first place.


Base::Vector3d dTrans(xTrans,yTrans,zTrans);
Base::Vector3d rotationCenter=this->getCenterData();
Base::Placement old_placement(getPositionData(),getRotationData());
// Code below allows for seamles rotation around axes without blocking on roll, pitch, yaw range ends.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: seamless

Base::Matrix4D new_placement_mat;
new_placement_mat = old_placement.toMatrix();
new_placement_mat.move(-rotationCenter);
if(dY!=0) new_placement_mat.rotZ(Base::toRadians(dY));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readability as mentioned above

if(dP!=0) new_placement_mat.rotY(Base::toRadians(dP));
if(dR!=0) new_placement_mat.rotX(Base::toRadians(dR));
new_placement_mat.move(rotationCenter);
new_placement_mat.move(dTrans);
Base::Placement new_placement(new_placement_mat);
new_placement.setPosition(old_placement.getPosition()+dTrans);
//setPlacementData(new_placement);//Does not work properly because of problems with euler angles. Angle values need to be adjusted below to fit into proper range.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What to do with this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I was trying to minimize count of UI updates but the first approach did not work properly.
It was supposed to be a note explaining why the rest of the algorithm was needed.
Can be removed or maybe rewritten in hopefully more clear way.
Which way would be preferred?


ui->xPos->setValue(new_placement.getPosition().x);
ui->yPos->setValue(new_placement.getPosition().y);
ui->zPos->setValue(new_placement.getPosition().z);

Base::Vector3d new_dir_vect;
double angle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor thing but our clang-tidy rules want initialized variables: double angle {};

new_placement.getRotation().getValue(new_dir_vect,angle);

// bug seen in freecad-0.21.2+dfsg1 (from debian source package) and in github master 20240708
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment must be reformatted because the maximum chars per line is 100.

// If there are issues with precision when rotating (360 deg rotation arround axis does not return to the same orientation) increase decimal places in Preferences (stored values in Gui::QuantitySpinBox are rounded and result in precision loss after multiple iterations of rotation)
// bug presence may be tested by uncomenting printf with "eventFilter-C" and "eventFilter-D" they will return different (RotXYZ) values when bug is present (D line will have visibly rounded values).
//printf("eventFilter-C\t(x,y,z)=(%f,%f,%f)\t(RotXYZ)=(%f,%f,%f)\t(RotAngle)=(%f)\n",new_placement.getPosition().x,new_placement.getPosition().y,new_placement.getPosition().z,new_dir_vect.x, new_dir_vect.y, new_dir_vect.z, Base::toDegrees<double>(angle));

ui->xAxis->setValue(new_dir_vect.x);
ui->yAxis->setValue(new_dir_vect.y);
ui->zAxis->setValue(new_dir_vect.z);
ui->angle->setValue(Base::toDegrees<double>(angle));
//printf("eventFilter-D\t(x,y,z)=(%f,%f,%f)\t(RotXYZ)=(%f,%f,%f)\t(RotAngle)=(%f)\n\n",ui->xPos->value().getValue(),ui->yPos->value().getValue(),ui->zPos->value().getValue(),ui->xAxis->value().getValue(),ui->yAxis->value().getValue(),ui->zAxis->value().getValue(),ui->angle->value().getValue());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove


// Euler angles (XY'Z'')
double Y,P,R;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialize

new_placement.getRotation().getYawPitchRoll(Y,P,R);
if(R>180) R-=360; else if(R<-180) R+=360;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readability

if(P>90) P-=180; else if(P<-90) R+=180;
if(Y>180) Y-=360; else if(Y<-180) Y+=360;
ui->yawAngle->setValue(Y);
ui->pitchAngle->setValue(P);
ui->rollAngle->setValue(R);

signalMapper->blockSignals(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using QSignalBlocker this line can be removed

onPlacementChanged(0);
return true;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

return false;
}

void Placement::open()
{
handler.openTransactionIfNeeded();
Expand Down
3 changes: 3 additions & 0 deletions src/Gui/Placement.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@
Base::Placement getPlacement() const;
void showDefaultButtons(bool);

bool eventFilter(QObject *, QEvent *ev);

Check warning on line 134 in src/Gui/Placement.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

'eventFilter' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]

Check warning on line 134 in src/Gui/Placement.h

View workflow job for this annotation

GitHub Actions / Lint / Lint

'eventFilter' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should declare it as: bool eventFilter(QObject* , QEvent* ev) override;


protected:
void changeEvent(QEvent *e) override;
void keyPressEvent(QKeyEvent*) override;
Expand All @@ -153,6 +155,7 @@
void setupSignalMapper();
void setupRotationMethod();
void bindProperty(const App::DocumentObject* obj, const std::string& propertyName);
void setupEventFilter();

bool onApply();
void setPlacementData(const Base::Placement&);
Expand Down
Loading