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: Fixes #10748 (Card 4: Elements should highlight) #11158

Closed
wants to merge 2 commits into from

Conversation

howie-j
Copy link
Contributor

@howie-j howie-j commented Oct 23, 2023

  • Added methods to change JCS pick properties
  • Joints are unpickable when creating new joints, pickable elsewhere
  • Lowered JCS transparency to make selection highlighting more visible

@github-actions github-actions bot added the WB Assembly Related to the Integrated Assembly Workbench label Oct 23, 2023
@chennes chennes self-assigned this Nov 6, 2023
@PaddleStroke
Copy link
Contributor

This is not exactly what I had in mind but probably good too. Perhaps you could make a short video and post it here so that other can weight in if they think it's the good way to go?

I build it before but don't remember exactly what it was doing. I will build again to see.

As a side note it will conflict with my card 9 PR. But I can rebase later.

@PaddleStroke
Copy link
Contributor

Actually a video is present here : #10748

@PaddleStroke
Copy link
Contributor

The JCS are pickable, but there is no highlighting on my side. Whether the joint is selected or not it looks the same:
image

@PaddleStroke
Copy link
Contributor

PaddleStroke commented Nov 7, 2023

So initially from the discussion we had during testing the previous PR we made 2 issues :

  • Card 3 : JCS visibility should toggle when not in edit :
  • Card 4 : When a joint is selected in the tree, its elements should highlight

So put together that meant :

  • When a joint is not selected the JCS are invisible. So it cannot be picked.
  • When a joint is selected in the tree : the JCS becomes visible, and the joint elements highlighted.

Now if we want to achieve that, making the JCS pickable is useless because they would be hidden until selected. Besides as they would be hidden until selected, highlighting them doesn't have any use. So in this context this PR would be useless.

Having said that perhaps our initial discussion was not ideal. So here the workflow you are offering is :

  • JCS are always visible.
  • JCS are pickable in the view (and in the tree) and they highlight the JCS of the joint when selected.

I guess more feedback from the community would help

@howie-j
Copy link
Contributor Author

howie-j commented Nov 7, 2023

The JCS are pickable, but there is no highlighting on my side. Whether the joint is selected or not it looks the same:

This is caused by non-optimal lightning, viewing angle and selection colors.

Changing the plane_sep() colors made this much better for me, try this:

material.diffuseColor.setValue([0.5, 0.5, 0.5])
material.ambientColor.setValue([0.5, 0.5, 0.5])
material.specularColor.setValue([0.5, 0.5, 0.5])
material.emissiveColor.setValue([0.5, 0.5, 0.5])
Screencast.from.2023-11-07.20-36-44.webm

@howie-j
Copy link
Contributor Author

howie-j commented Nov 7, 2023

I can try to explain this PR a bit better 😀
These are the cards:

Card 3:

  • JCS become hidden when their edition is left.
  • JCS become visible when you select the joint in the tree view. And hides when the joint is not selected anymore.

Card 4:

  • When a joint is not being edited, if you select it, the selected elements >(face, edge or vertex) should be highlighted.

Hiding the joint when edit mode is closed is trivial, just set visibility to false.

All other objects in freecad highlights when selected in the tree, so i could not figure out why joints would not highlight, until i saw they were deliberately set unpickable (to make selection in edit mode possible). Setting them pickable felt very natural to me from a UX perspective, to be able to double click on them to edit, click to hide, select them in tree view to highlight them, select to delete etc.

I also discovered that highlighting the elements looked a bit "messy", say if a large face is highlighed you wouldn't know where exactly on the face the JCS were connected, and hidden edges would not show. Example:

Elements highlighted:
Element highlighted

JCS highlighted:
JCS highlighted

So in my opinion, highlighting the JCS's looked better and felt better (highly subjective of course). Another point is that when you double click a JCS and enter edit mode, the elements will highlight, so that's a simple option if you need to see which element the JCS is connected to.

After that, my plan to solve these cards were the following:

  1. Fix the selection and highlighting issue to make joints selectable (this pr)
  2. Make sure 3d selection highlighting and tree view selection works both ways (this pr)
  3. Hide joints automatically after they are created (another card 3 pr)
  4. Try to solve the show joint and highlight on treeview selection part. If i could not do that, then create a Show all joints/hide all joints command and toolbar button as a temporary solution until someone else can write the observer properly.

Now if we want to achieve that, making the JCS pickable is useless because they would be hidden until selected. Besides as they would be hidden until selected, highlighting them doesn't have any use. So in this context this PR would be useless.

That could be, but in my experience with other cad systems such as onshape, it can be very beneficial when working with large assemblies to show all joints, and then select them in the 3d view to find the correct one to edit/delete/modify.

Feedback is very much welcome, and i guess when the solver is working and merged there will be more than enough feedback :)

@PaddleStroke
Copy link
Contributor

PaddleStroke commented Nov 8, 2023

Your explications are convincing for me, and the workflow you describe seems good.

So with your workflow the point 4 :

Try to solve the show joint and highlight on treeview selection part. If i could not do that, then create a Show all joints/hide all joints command and toolbar button as a temporary solution until someone else can write the observer properly.

Does not feel necessary. We can just hide the JCS when creation is done. Then let the user handle the visibility of it like he would for any other Object.

@PaddleStroke
Copy link
Contributor

And indeed you are right, with double click now (in card9 pr) you start edition of the joint and the elements are highlighted.

@howie-j
Copy link
Contributor Author

howie-j commented Nov 8, 2023

Try to solve the show joint and highlight on treeview selection part. If i could not do that, then create a Show all joints/hide all joints command and toolbar button as a temporary solution until someone else can write the observer properly.

Does not feel necessary. We can just hide the JCS when creation is done. Then let the user handle the visibility of it like he would for any other constraint.

Sounds good 👍 These small details and workflow optimalizations are probably going to be much easier to figure out once we can start making assemblies and using the workbench, so probably not necessary to spend too much time on before we know if they will be needed or not.

@PaddleStroke
Copy link
Contributor

@howie-j so we wait until the card9 PR merges to m erge this one right? Or you prefer to merge this now?

@howie-j
Copy link
Contributor Author

howie-j commented Nov 20, 2023

@PaddleStroke Yep we can absolutely wait, card9 is much more important. And this one is small, I can rebase as soon as card 9 is merged 👍

@chennes chennes added the ✋ On hold This PR must not be merged before some condition is met label Nov 22, 2023
@PaddleStroke
Copy link
Contributor

@howie-j do you want me to cherry-pick these changes into the card9 branch so that we can close this PR?

@howie-j
Copy link
Contributor Author

howie-j commented Dec 18, 2023

@PaddleStroke That would be great! I can probably rebase on card 9 and resolve the merge conflicts if you want, but i won't have the time before this weekend the earliest. If you want to do the rebase please go ahead :)

@PaddleStroke
Copy link
Contributor

@howie-j I am merging your changes in card9 now.

One remark/question : double clicking on a JCS in the 3d view does not open the task for the joint edition, as double clicking in the tree does. Instead is selects the whole assembly. Do you know how to do this already?

@howie-j
Copy link
Contributor Author

howie-j commented Jan 8, 2024

One remark/question : double clicking on a JCS in the 3d view does not open the task for the joint edition, as double clicking in the tree does. Instead is selects the whole assembly. Do you know how to do this already?

Hmm, no i don't think i know why this happenes. When i worked on this PR last, the double click to edit joint feature did not exist.

If you hide the assembly model, but keeps the joint visible, does double clicking work then?

@PaddleStroke
Copy link
Contributor

Nah same happen. It's not the same function that gets triggered on view double clicked than on tree double click I guess. I need to look in more details.

@PaddleStroke
Copy link
Contributor

I cherry picked the changes to card9, this can be closed

@sliptonic
Copy link
Member

Per @PaddleStroke, closing

@sliptonic sliptonic closed this Jan 15, 2024
@howie-j howie-j deleted the asm_card_4 branch January 15, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✋ On hold This PR must not be merged before some condition is met WB Assembly Related to the Integrated Assembly Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants