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

Assembly joints #10611

Merged
merged 3 commits into from Sep 19, 2023
Merged

Conversation

PaddleStroke
Copy link
Contributor

@PaddleStroke PaddleStroke commented Sep 8, 2023

This is a basic implementation of a constraint system for the assembly wb.

The approach is the one that uses 'joint connectors'. It is a similar approach as A4 but the idea is to generate the placement based on the user selection. So for example you select the plane face4 of your body, the joint-connector placement is going to be the rotation of that plane, and the position of the face4 vertex that is the closer to your mouse. It is also similar to the approach of two well known commercial package.

The reason of this choice is that the solver developped by Dr Koh is following this logic to build assembly. Also this workflow is good and intuitive.

  • 'Create Assembly' also create a 'joints' folder.
  • Adds 'create joint xxx' commands. These commands prompt the user to select 2 features of 2 different parts/bodies.
  • The command creates a 'joint' object that stores the selected objects and selected elements and generated placement in properties.

The joint-connectors are ready to be assembled. At this stage we now need to have the API of the solver ready and accessible to move forward and actually solve the assembly.

Note:

@github-actions github-actions bot added Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB Assembly Related to the Integrated Assembly Workbench labels Sep 8, 2023
@onekk
Copy link

onekk commented Sep 11, 2023

Keep in mind scriptability, as GUI is not the only way of doing things.

Usually good joints are axes, vertexes and even some middle points, like a circular face center, as frequently you have some "degree of freedom" determined by "ball joints", bearing and pins.

It could be interesting to derive some feature lengths from already existing properties, as "circle center" of "middle point of a line" or "middle of an arc".

Regards

Carlo D.

@PaddleStroke PaddleStroke force-pushed the Assembly_joint_basis branch 2 times, most recently from 8425483 to 53e2071 Compare September 14, 2023 15:04
@PaddleStroke
Copy link
Contributor Author

This is now ready for review.

@howie-j
Copy link
Contributor

howie-j commented Sep 14, 2023

Non-code feedback

Note: I've mostly tested the workflow of adding joints, since that is what this PR adresses.

I find it very intuitive and easy to add joints, really great work so far. I also like the agile approach of making small PR's, quickly merging, getting early feedback and solving problems continually.

👍 👍 👍

  • The LCS indicator snaps to edges, vertexes, faces and holes mostly as expected, and the preview is awesome
  • Nice to be able to edit joint type in task view, and later in property view
  • Quick to hide LCS's/joints by clicking "Joints" folder and space bar
  • Overall i'm very impressed by the user experience so far. Not only miles ahead of the other wb's, but also on par with commercial systems.

👎

  • The LCS's / joints are quite big and intrusive, and should perhaps be hidden automatically after a joint is created
  • Enter should confirm the joint, to be consistent with task panels (PD Pad, Pocket etc)
  • Double clicking on a joint in tree view should launch the joint task panel for easy editing
  • No way of selecting and removing a joint connector in the task panel (by right clicking in the list for example)
  • Selecting a joint connector in the task panel list and pressing delete will delete the last feature of the part, not remove the joint connector
  • If you delete the "Joints" folder/group, you get an error when attempting to create a new joint. The create joint command should check if a Joints folder exists, and create a new one if not
  • Joint/LCS snap has room for improvement, for example snap to mid line and center of face (nitpicking, not important atm)
  • (Very personal opinion) The shortcuts follows the horrible sketcher model of multiple keys, when Assembly currently only has 10 commands. In my opinion it's better to only create default shortcuts for the most used commands, and let the user themself create shortcuts for less used commands. Much better to have 5 or 10 single key shortcuts that the user might remember, than 30 that the user wont.. :)

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Sep 15, 2023

Thank you guys for the review.

1 - [ ] The LCS's / joints are quite big and intrusive, and should perhaps be hidden automatically after a joint is created
2 - [ ] Enter should confirm the joint, to be consistent with task panels (PD Pad, Pocket etc)
3 - [ ] Double clicking on a joint in tree view should launch the joint task panel for easy editing
4 - [ ] No way of selecting and removing a joint connector in the task panel (by right clicking in the list for example)
5 - [ ] Selecting a joint connector in the task panel list and pressing delete will delete the last feature of the part, not remove the joint connector
6 - [x] If you delete the "Joints" folder/group, you get an error when attempting to create a new joint. The create joint command should check if a Joints folder exists, and create a new one if not
7 - [ ] Joint/LCS snap has room for improvement, for example snap to mid line and center of face (nitpicking, not important atm)
8 - [x] (Very personal opinion) The shortcuts follows the horrible sketcher model of multiple keys, when Assembly currently only has 10 commands. In my opinion it's better to only create default shortcuts for the most used commands, and let the user themself create shortcuts for less used commands. Much better to have 5 or 10 single key shortcuts that the user might remember, than 30 that the user wont.. :)

  • 1, 3, 4, 7 : Yes I agree. But each of them need more work. I was thinking that these could be subsequent issues / PR, as to involve more people in the development.

  • 5 : Yes this is an undefined behavior. I think as delete is undefined in the list, the event goes back to the parent, until it reaches the comboview, and there it does the tree delete action. I would say it goes with 4.

  • 6 and 8 done

  • 2 : When you want to catch key press events in the 3D view, you need to use addEventCallback:

        self.callbackKey = self.view.addEventCallback("SoKeyboardEvent", self.KeyboardEvent)
        self.view.removeEventCallback("SoLocation2Event", self.callbackKey)

    def KeyboardEvent(self, info):
        if info["State"] == "UP" and info["Key"] == "ESCAPE":
            # Create a qt key release event for the Escape key
            key_release_event = QtGui.QKeyEvent(
                QtGui.QKeyEvent.KeyPress, QtCore.Qt.Key_Escape, QtCore.Qt.NoModifier
            )
            # Send the key release event to the taskbox
            QtGui.QApplication.sendEvent(self.form, key_release_event)

        if info["State"] == "UP" and info["Key"] == "RETURN":
            if self.accept():
                Gui.Control.closeDialog()

The question is how to close the task. I found the 2 options above, first make a QKeyEvent to act as if user pressed esc in the taskbox and send it to the taskbox. Second call reject or accept, and if true then closeDialog.
@chennes could you please advise what is the proper way to dismiss a task when the 3d view has the focus?

Besides I found a weird bug with this. Both work only ONCE but if you start the tool a second time again and try press enter or escape (when view has focus), then it crash.

@qewer33
Copy link
Contributor

qewer33 commented Sep 15, 2023

I also just gave the branch a test and I really like it! The LCS selection is incredibly intuitive. I agree with the points mentioned by howie-j but I'd like to add a few little things:

  • I realized that some of the joint icons have improper padding making them appear small on the toolbar, that's my bad. I can fix it in a PR later when this is merged or you can also address it in this PR if you want @PaddleStroke
  • It would be great to have alignment options for the part LCSs in the task view (I'm thinking like two groups, "Edit First Part LCS" and "Edit Second Part LCS" with options to rotate, flip, offset etc.). This might be better if it's implemented after this base PR is merged though, but would love to see that
  • When will the Ondsel Solver be added so we can test the actual solver joint functionality?
  • howie-j mentioned how LCSs would be better hidden, to add on that, it would be great if the LCSs and their attached vertices/features can be highlighted when the joint is selected in the tree view

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Sep 15, 2023

@qewer33

  • Please go ahead and make a PR, as it's separated there won't be conflicts. (I don't see them on my end, dunno why the qrc is not being recompiled, I probably need a full rebuilt.)
  • Alignment options to rotate / reverse the placement is something we need to implement indeed.
  • I'm not sure when the API will be ready. Hopefully soon !
  • Yes that's needed too

@chennes
Copy link
Member

chennes commented Sep 15, 2023

could you please advise what is the proper way to dismiss a task when the 3d view has the focus?

I think that in this case both of your suggestions are reasonable. I normally don't care for generating keypress events, but since yours is really passing along a keypress event it seems like an acceptable path to me. I'd personally probably take the other route, and call accept or reject, but I don't feel strongly about it.

@PaddleStroke
Copy link
Contributor Author

Ok then I'll go with

  if self.accept():
      Gui.Control.closeDialog()

Are you guys able to reproduce the crash I'm having when closing the tool with Escape for the second time ? If so do you have any logs on your side that could hint at what is wrong? I have no message on my end.

@howie-j
Copy link
Contributor

howie-j commented Sep 16, 2023

Yes, esc crashes freecad.
Steps to reproduce:

  • Switch to assembly
  • Create new assembly
  • Click create joint (any)
  • Click in the 3D view
  • Double click escape key, freecad crashes

FreeCAD.log

@chennes
Copy link
Member

chennes commented Sep 17, 2023

That looks like it's trying to send an event to a Widget that has been deleted.

@jpg87Fr
Copy link

jpg87Fr commented Sep 18, 2023

Hello everyone.
I would like to know what to do to test the progress of the new Assembly WB.

@qewer33
Copy link
Contributor

qewer33 commented Sep 18, 2023

Hello everyone. I would like to know what to do to test the progress of the new Assembly WB.

You can clone the branch of this PR and build it but it currently doesn't have the solver integrated, you may want to test it after that's done.

@chennes
Copy link
Member

chennes commented Sep 18, 2023

I would like to know what to do to test the progress of the new Assembly WB.

Right now there's not much to see -- are you interested in testing the un-merged PR code, or the code that's already been integrated into the master branch?

@chennes chennes marked this pull request as draft September 18, 2023 16:06
@chennes
Copy link
Member

chennes commented Sep 18, 2023

I'm flagging as draft again until the crash is resolved.

@PaddleStroke PaddleStroke marked this pull request as ready for review September 19, 2023 08:51
@PaddleStroke
Copy link
Contributor Author

I just forced push with :

  • Fixing the crash. For reference, it was a copy paste error in the removeEventCallback of the keyboard event:
        self.view.removeEventCallback("SoLocation2Event", self.callbackMove)
        self.view.removeEventCallback("SoLocation2Event", self.callbackKey)

instead of

        self.view.removeEventCallback("SoLocation2Event", self.callbackMove)
        self.view.removeEventCallback("SoKeyboardEvent", self.callbackKey)
  • Improve the tooltips of the joint commands.

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Sep 19, 2023

A summary of what needs to be done in subsequent PRs:

  • JCS are ugly.
  • JCS should scale with zoom.
  • JCS should hide when the joint is not in edit.
  • When Joint is selected in the tree, the JCS should be visible.
  • When Joint is selected in the tree, the selected features should be highlighted.
  • Joints should be editable. Double clicking should open the Task
  • Delete a joint connector in the taskbox list should remove it from selection.
  • JCS should also snap to line middle and face center of gravity.
  • Add buttons to rotate / reverse JCS

If any of you want to pick up one of these let me know !
@qewer33 maybe you'd be interested to make a better design for the JCS? You can just make a mockup and I'll try to make it in coin. Or if you want to do it in coin directly you are more than welcome 😄

@sliptonic
Copy link
Member

A summary of what needs to be done in subsequent PRs:

Awesome breakdown. These should be moved to separate issues and added to the project.
Once this PR is merged, discussion will move on and nobody will see these here.

@chennes
Copy link
Member

chennes commented Sep 19, 2023

OK, this looks ready to me then, I'm going to merge it.

@chennes chennes merged commit fec8888 into FreeCAD:master Sep 19, 2023
7 checks passed
@qewer33
Copy link
Contributor

qewer33 commented Sep 19, 2023

If any of you want to pick up one of these let me know ! @qewer33 maybe you'd be interested to make a better design for the JCS? You can just make a mockup and I'll try to make it in coin. Or if you want to do it in coin directly you are more than welcome 😄

Personally, I find the current LCS design usable. Just checked OnShape and Fusion360 and they seem to have a very similar graphic. They will definitely look better a little smaller though, can you test that?

@qewer33
Copy link
Contributor

qewer33 commented Sep 19, 2023

Ok I couldn't hold myself and made a mockup anyway @PaddleStroke 😅

It's similar to the current one, just a little shorter and thicker lines with a translucent plane to show the local base XY plane.

image

Here's what OnShape has for reference but I don't think it's that great. Though it might be better to show the translucent plane on the top mockup at the center of the LCS, like here.

image

@jpg87Fr
Copy link

jpg87Fr commented Sep 19, 2023

> are you interested in testing the un-merged PR code, or the code that's already been integrated into the master branch?
I don't know anything about code. I will wait until new elements are present in the Weekly version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD WB Assembly Related to the Integrated Assembly Workbench
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants