Integration with 3Dconnexion input devices #1311

pskowronskiTDx · 2023-06-14T13:56:20Z

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file.

I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

ghutchis

In general, these seem reasonable. I'd much, much rather see new API methods and implementations in their associated files than a tdxextensions.cpp with some miscellaneous methods that belong elsewhere.

The main ifdef pieces I'd want to see are relative to drawing icons, etc.

avogadro/rendering/camera.cpp

avogadro/rendering/camera.h

avogadro/rendering/geometryvisitor.cpp

avogadro/rendering/geometryvisitor.h

avogadro/rendering/glrenderer.cpp

avogadro/rendering/scene.h

avogadro/rendering/tdxextensions.cpp

github-actions · 2023-06-20T22:22:01Z

Here are the build results
Avogadro2.AppImage
macOS.dmg
Ubuntu-Latest.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

ghutchis · 2023-06-30T18:37:03Z

@pskowronskiTDx - any comments / concerns about the requested changes? Happy to discuss over e-mail if that's easier. Thanks!

pskowronskiTDx · 2023-07-05T09:20:14Z

@ghutchis - Here is a summary of changes I did according to your review. I hope you wil be happy with them.

The definitions of new methods have been putted back to relevant source files, tdxextensions.cpp has been removed.
The #ifdef conditions got removed for Scene, Camera and GLRenderer (except for part responsible for drawing the 3Dx icon) classes.
Saving the geometry got reduced to spheres. The point of doing so is to access the current collection of the spheres visible on scene. It is needed to calculate bounding box of the whole molecule or it's part consisting of selected atoms. I couldn't come up with more efficient solution to this problem so far. Maybe there is another approach you can suggest to follow?
Sources were cleaned-up of whitespace changes and unnecessary includes.

Signed-off-by: Patryk Skowroński <pskowronski@3dconnexion.com>

ghutchis · 2023-07-05T13:36:41Z

I'm traveling today, but will take a look. @cryos - do you have a moment to look as well?

github-actions · 2023-07-05T15:30:59Z

Here are the build results
Avogadro2.AppImage
macOS.dmg
Ubuntu-Latest.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

pskowronskiTDx · 2023-07-13T11:24:53Z

Hi Geoff, do you managed to take a look on the changes I have proposed to both PRs? Thanks.

ghutchis · 2023-07-26T16:28:02Z

Saving the geometry got reduced to spheres. The point of doing so is to access the current collection of the spheres visible on scene. It is needed to calculate bounding box of the whole molecule or it's part consisting of selected atoms. I couldn't come up with more efficient solution to this problem so far. Maybe there is another approach you can suggest to follow?

Can't you calculate the bounding box (or selected atom bounding) in core/molecule.* or qtgui/molecule.* instead? There are a few places that use similar code.

ghutchis · 2023-07-26T16:28:40Z

Please just run clang-format on your modified files, thanks.

ghutchis

This looks much better, but please avoid using "API Extension" comments, which aren't needed and let's consider putting the bounding box into the Molecule class instead.

ghutchis · 2023-07-26T16:30:01Z

avogadro/rendering/camera.h

@@ -121,6 +121,20 @@ class AVOGADRORENDERING_EXPORT Camera
   */
  void calculatePerspective(float fieldOfView, float zNear, float zFar);

+  /**
+   * << API Extension for TDX >>


Please omit this. You may have added it, but there's plenty of reason to use this in other code.

ghutchis · 2023-07-26T16:31:47Z

avogadro/rendering/geometryvisitor.cpp

@@ -187,4 +189,55 @@ void GeometryVisitor::average()
  }
 }

+void GeometryVisitor::boundingBox(double& minX, double& minY, double& minZ,


As I mentioned, it seems easier to put this into core/molecule.*. If there's some reason you can't access the Molecule class, this is okay.

ghutchis · 2023-07-26T16:32:47Z

avogadro/rendering/geometryvisitor.h

@@ -62,6 +64,22 @@ class GeometryVisitor : public Visitor
   */
  float radius();

+  /**
+   * <<API Extension for TDX>>


Please, please omit "API Extension." I'm more than happy to provide credit / acknowledgement for your work and TDX, but we don't need to clutter the documentation with who wrote particular methods.

ghutchis · 2023-07-26T16:33:47Z

avogadro/rendering/scene.cpp

+                           double& maxX, double& maxY, double& maxZ,
+                           const std::vector<bool>& flags)
+{
+  GeometryVisitor visitor;


As above - does this need to be in the Scene class?

ghutchis · 2023-07-26T16:34:05Z

avogadro/rendering/scene.h

@@ -131,10 +131,26 @@ class AVOGADRORENDERING_EXPORT Scene
  /** Clear the scene of all elements. */
  void clear();

+  /**
+   * <<API Extension for TDX>>


Please remove this line.

ghutchis · 2023-07-26T16:35:19Z

avogadro/rendering/scene.h

+   * @param flags [in] flags informing which atoms will be included
+   * in the bounding box
+   */
+  void getBoundingBox(double& minX, double& minY, double& minZ, double& maxX,


Again, I think it would be easier to access the bounding box directly from the molecule (which could adjust as needed for isosurface meshes, etc.)

cryos · 2023-07-27T15:38:39Z

I have been really busy the last month, sorry. I can take a look, you covered a lot of my initial concerns with comments and where the additional API is best placed @ghutchis

pskowronskiTDx · 2023-07-31T14:38:17Z

Thank you for advicing to move bounding box calculation to the Molecule class. Indeed, this is much more elegant. I have also removed mentioned lines from comments. Clang-format was applied on my code, but I can see there are still some issues...

Signed-off-by: Patryk Skowroński <pskowronski@3dconnexion.com>

ghutchis · 2023-08-07T19:14:21Z

avogadro/core/molecule.cpp

+
+  bool noSelection = true;
+
+  for (uint32_t i = 0; i < m_selectedAtoms.size(); i++) {


For what it's worth, you can get this from isSelectionEmpty() but it's not a big deal either way.

ghutchis

This all looks great .. just some minor nitpicks in the bounding box.

ghutchis · 2023-08-07T19:15:21Z

avogadro/core/molecule.cpp

+
+  for (uint32_t i = 0; i < atomCount(); i++) {
+    if (noSelection || m_selectedAtoms[i]) {
+      double radius = 1.0;


This seems small. I'd suggest adding it as a parameter with a default of 1.0?

ghutchis · 2023-08-07T19:17:20Z

avogadro/core/molecule.cpp

+      Vector3 boxMinBuffer;
+      Vector3 boxMaxBuffer;
+
+      boxMinBuffer.x() = atom(i).position3d().x() - radius;


boxMinBuffer = atom(i).position3d().array() - radius;

https://stackoverflow.com/a/35693493/131896

ghutchis · 2023-08-07T19:17:46Z

avogadro/core/molecule.cpp

+      boxMinBuffer.x() = atom(i).position3d().x() - radius;
+      boxMinBuffer.y() = atom(i).position3d().y() - radius;
+      boxMinBuffer.z() = atom(i).position3d().z() - radius;
+      boxMaxBuffer.x() = atom(i).position3d().x() + radius;


boxMaxBuffer = atom(i).position3d().array() + radius;

This way Eigen will properly perform an element-wide operation.

ghutchis · 2023-08-07T19:19:04Z

avogadro/core/molecule.cpp

+      boxMaxBuffer.z() = atom(i).position3d().z() + radius;
+
+      if (boxMinBuffer.x() < boxMin.x())
+        boxMin.x() = boxMinBuffer.x();


boxMin.x() = min(boxMin.x(), boxMinBuffer.x());

etc. .. which is more readable, since the library handles the conditional

avogadro/core/molecule.h

Signed-off-by: Patryk Skowroński <pskowronski@3dconnexion.com>

github-actions · 2023-08-10T14:44:31Z

Here are the build results
Avogadro2.AppImage
macOS.dmg
Ubuntu-Latest.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

ghutchis · 2023-08-10T14:53:52Z

Looks good to me - I'll re-run clang-format when I get a chance. Thanks for sticking with this, I'll turn on the flag for Windows and Mac builds.

welcome · 2023-08-10T14:54:04Z

Congrats on merging your first pull request! 🎉 Thanks for making Avogadro better for everyone!

github-actions · 2023-08-10T16:35:02Z

Here are the build results
Avogadro2.AppImage
macOS.dmg
Ubuntu-Latest.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

ghutchis requested changes Jun 20, 2023

View reviewed changes

Patryk Skowroński added 2 commits July 5, 2023 12:56

Added extensions for 3Dconnexion integration

b5a07b1

Signed-off-by: Patryk Skowroński <pskowronski@3dconnexion.com>

Changes after 1st PR review

d30019f

Signed-off-by: Patryk Skowroński <pskowronski@3dconnexion.com>

pskowronskiTDx force-pushed the tdx-integration-pr branch from 52775f9 to d30019f Compare July 5, 2023 10:56

ghutchis requested changes Jul 26, 2023

View reviewed changes

Bounding box calucation moved to Molecule.

6009616

Fix formatting

7461add

Signed-off-by: Patryk Skowroński <pskowronski@3dconnexion.com>

ghutchis reviewed Aug 7, 2023

View reviewed changes

ghutchis requested changes Aug 7, 2023

View reviewed changes

Molecule::boundingBox refactor

a914e8d

Signed-off-by: Patryk Skowroński <pskowronski@3dconnexion.com>

ghutchis added the enhancement feature changes / API changes label Aug 10, 2023

ghutchis merged commit b5239fe into OpenChemistry:master Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration with 3Dconnexion input devices #1311

Integration with 3Dconnexion input devices #1311

pskowronskiTDx commented Jun 14, 2023

ghutchis left a comment

github-actions bot commented Jun 20, 2023

ghutchis commented Jun 30, 2023

pskowronskiTDx commented Jul 5, 2023 •

edited

Loading

ghutchis commented Jul 5, 2023

github-actions bot commented Jul 5, 2023

pskowronskiTDx commented Jul 13, 2023

ghutchis commented Jul 26, 2023

ghutchis commented Jul 26, 2023

ghutchis left a comment

ghutchis Jul 26, 2023

ghutchis Jul 26, 2023

ghutchis Jul 26, 2023

ghutchis Jul 26, 2023

ghutchis Jul 26, 2023

ghutchis Jul 26, 2023

cryos commented Jul 27, 2023

pskowronskiTDx commented Jul 31, 2023

ghutchis Aug 7, 2023 •

edited

Loading

ghutchis left a comment

ghutchis Aug 7, 2023

ghutchis Aug 7, 2023

ghutchis Aug 7, 2023

ghutchis Aug 7, 2023

github-actions bot commented Aug 10, 2023

ghutchis commented Aug 10, 2023

welcome bot commented Aug 10, 2023

github-actions bot commented Aug 10, 2023


		bool noSelection = true;

		for (uint32_t i = 0; i < m_selectedAtoms.size(); i++) {

Integration with 3Dconnexion input devices #1311

Integration with 3Dconnexion input devices #1311

Conversation

pskowronskiTDx commented Jun 14, 2023

ghutchis left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 20, 2023

ghutchis commented Jun 30, 2023

pskowronskiTDx commented Jul 5, 2023 • edited Loading

ghutchis commented Jul 5, 2023

github-actions bot commented Jul 5, 2023

pskowronskiTDx commented Jul 13, 2023

ghutchis commented Jul 26, 2023

ghutchis commented Jul 26, 2023

ghutchis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryos commented Jul 27, 2023

pskowronskiTDx commented Jul 31, 2023

ghutchis Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

ghutchis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 10, 2023

ghutchis commented Aug 10, 2023

welcome bot commented Aug 10, 2023

github-actions bot commented Aug 10, 2023

pskowronskiTDx commented Jul 5, 2023 •

edited

Loading

ghutchis Aug 7, 2023 •

edited

Loading