-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,30 +47,35 @@ | |
#include <Gui/Window.h> | ||
|
||
#include "Placement.h" | ||
#include "ui_Placement.h" | ||
|
||
#include <Gui/MainWindow.h> | ||
#include <Gui/View3DInventor.h> | ||
#include <Inventor/SbVec3f.h> | ||
#include <Gui/SpaceballEvent.h> | ||
#include <Base/DualQuaternion.h> | ||
|
||
using namespace Gui::Dialog; | ||
namespace sp = std::placeholders; | ||
|
||
namespace Gui { namespace Dialog { | ||
class find_placement | ||
{ | ||
public: | ||
explicit find_placement(const std::string& name) : propertyname(name) | ||
{ | ||
} | ||
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)) | ||
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)) | ||
return false; | ||
} | ||
return elem.second->isDerivedFrom | ||
|
@@ -83,11 +88,11 @@ | |
static App::PropertyPlacement* getProperty(const App::DocumentObject* obj, | ||
const std::string& propertyName) | ||
{ | ||
std::map<std::string,App::Property*> props; | ||
obj->getPropertyMap(props); | ||
|
||
// search for the placement property | ||
std::map<std::string,App::Property*>::iterator jt; | ||
jt = std::find_if(props.begin(), props.end(), find_placement(propertyName)); | ||
if (jt != props.end()) { | ||
return dynamic_cast<App::PropertyPlacement*>(jt->second); | ||
|
@@ -179,7 +184,7 @@ | |
void PlacementHandler::openTransaction() | ||
{ | ||
App::Document* activeDoc = App::GetApplication().getActiveDocument(); | ||
if (activeDoc) | ||
activeDoc->openTransaction("Placement"); | ||
} | ||
|
||
|
@@ -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; | ||
list.insert(list.begin(), objs.begin(), objs.end()); | ||
return list; | ||
} | ||
|
@@ -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; | ||
list.reserve(selectionObjects.size()); | ||
for (const auto& it : selectionObjects) { | ||
const App::DocumentObject* obj = it.getObject(); | ||
|
@@ -228,7 +233,7 @@ | |
|
||
void PlacementHandler::revertTransformationOfViewProviders(Gui::Document* document) | ||
{ | ||
std::vector<const App::DocumentObject*> obj = getObjects(document); | ||
for (const auto & it : obj) { | ||
auto property = find_placement::getProperty(it, this->propertyName); | ||
if (property) { | ||
|
@@ -462,10 +467,12 @@ | |
setupUnits(); | ||
setupSignalMapper(); | ||
setupRotationMethod(); | ||
setupEventFilter(); | ||
} | ||
|
||
Placement::~Placement() | ||
{ | ||
if (qApp!=NULL) qApp->removeEventFilter(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Could you check if you can trigger it by something like this:
After that procedure i can get different segmentation faults.
or
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
@@ -547,6 +554,12 @@ | |
ui->stackedWidget->setCurrentIndex(index); | ||
} | ||
|
||
|
||
void Placement::setupEventFilter() | ||
{ | ||
if (qApp!=NULL) qApp->installEventFilter(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
} | ||
|
||
void Placement::showDefaultButtons(bool ok) | ||
{ | ||
ui->buttonBox->setVisible(ok); | ||
|
@@ -559,6 +572,83 @@ | |
} | ||
} | ||
|
||
bool Placement::eventFilter(QObject*, QEvent* ev) { | ||
//handle spacenav daemon motion events in placement window | ||
if (//event->type() == Spaceball::ButtonEvent::ButtonEventType || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To improve readability you should write:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or simply
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! My bad!!! You are quite correct! |
||
signalMapper->blockSignals(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recommend to use: |
||
// 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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed commented code There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What to do with this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A minor thing but our clang-tidy rules want initialized variables: |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove |
||
|
||
// Euler angles (XY'Z'') | ||
double Y,P,R; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When using QSignalBlocker this line can be removed |
||
onPlacementChanged(0); | ||
return true; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation |
||
return false; | ||
} | ||
|
||
void Placement::open() | ||
{ | ||
handler.openTransactionIfNeeded(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 GitHub Actions / Lint / Lint
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should declare it as: |
||
|
||
protected: | ||
void changeEvent(QEvent *e) override; | ||
void keyPressEvent(QKeyEvent*) override; | ||
|
@@ -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&); | ||
|
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.
This header seems to be superfluous
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.
Yes, every thing except
#include <Gui/SpaceballEvent.h>
seems superfluous and are left from old experiments.