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

[Problem] Automatic naming is confused/wrong with the grounded Symbol of the Assembly Workbench #12546

Closed
1 task done
Tracked by #12907
tobiasfalk opened this issue Feb 22, 2024 · 23 comments · Fixed by #12406
Closed
1 task done
Tracked by #12907
Labels
Bug This issue or PR is related to a bug WB Assembly Related to the Integrated Assembly Workbench

Comments

@tobiasfalk
Copy link
Contributor

tobiasfalk commented Feb 22, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Problem description

While testing the Assembly workbench in the latest Weekly Build I created two cubes("Cube" and "Cube001" as normal) and then grounded one, after that I created a third cube which was not named "Cube002" but just "Cube". I assume this is because of the Lock symbol in the name of the first cube.
Unbenannt

Full version info

OS: Windows 10 build 19045
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.36082 (Git)
Build type: Release
Branch: main
Hash: 7a025e4e60a607bdbdd1599627ce13bb8eeda071
Python 3.10.13, Qt 5.15.8, Coin 4.0.2, Vtk 9.2.6, OCC 7.6.3
Locale: German/Germany (de_DE)
Installed mods: 
  * 3DfindIT 1.2.0
  * A2plus 0.4.56a
  * AirPlaneDesign 0.4.0
  * Assembly3 0.12.0
  * Assembly4 0.11.10
  * CADExchanger
  * cadquery_module
  * CurvedShapes 1.0.5
  * dxf-library
  * DynamicData 2.46.0
  * ExplodedAssembly
  * fasteners 0.5.2
  * fcgear 1.0.0
  * FeedsAndSpeeds 0.5.0
  * FEM_FrontISTR 0.1.0
  * FreeCAD_Electric
  * FreeCAD_Schematics
  * GDML 2.0.0
  * kicadStepUpMod 10.11.7
  * Plot 2022.4.17
  * pyrate
  * Render 2022.2.0
  * sheetmetal 0.2.56
  * Ship 2022.4.11
  * SteelColumn
  * ThreadProfile 1.82.0
  * timber
  * trails 2022.1.0


### Subproject(s) affected?

Assembly

### Anything else?

_No response_

### Code of Conduct

- [X] I agree to follow this project's Code of Conduct
@maxwxyz maxwxyz added Bug This issue or PR is related to a bug WB Assembly Related to the Integrated Assembly Workbench labels Feb 22, 2024
@maxwxyz
Copy link
Collaborator

maxwxyz commented Feb 22, 2024

@PaddleStroke

@PaddleStroke
Copy link
Contributor

Yes the lock is in the label. So it's the same as if you create one cube, rename it to 'Banana' then create a new cube. The second cube will be called 'Cube' because the name is free.
So this is not really a bug, and in real life I'm not sure it would be a real annoyance.
Besides I don't see an easy fix here.

@tobiasfalk
Copy link
Contributor Author

I like the Lock but I do not like it to be part of the lable, I think this will confuse some in the future.
There are multiple ways to solve this

  • Adding the Lock to the Symbol(on top.of it), this way the Lock has nothing to do with the lable

  • Adding the Lock only when the TreeView is created, this way the lable remains the lable and the Lock is a separated part of the Text in the TreeView

  • Check for the lock in the automating code and everywhere the lable is changed(not my favourite)

@pierreporte
Copy link

@PaddleStroke Then it sounds like to be related to #12139. The problem of names in the tree is wider (see also #12141).

@PaddleStroke
Copy link
Contributor

Yes it's not ideal that the lock is in the label indeed.
Maybe it should be an icon. Linkstage has a ton of icons, but to the point where it is confusing. The icon on the right side seemed nice.

When you say 'on top of the symbol' you mean on top of the object icon?

@PaddleStroke
Copy link
Contributor

PaddleStroke commented Feb 26, 2024

@wwmayer is there a way to overlay an icon on top of any object icon? So that the lock icon can be overlayed on top of other icons :
image

I had a look at how bodies are doing the tip indicator, but they are using the PartDesign::ViewProvider class that reimplement the mergeColorfulOverlayIcons function.

The thing is that here I want to add an overlay to any kind of VP so I'm not sure how to handle this.

I was thinking about setting the overlayed icon in the ViewProviderAssembly like this :

void AssemblyGui::ViewProviderAssembly::setLockIcons()
{
    auto* assemblyPart = static_cast<AssemblyObject*>(getObject());
    std::vector<App::DocumentObject*> groundedJoints = assemblyPart->getGroundedJoints();
    std::vector<App::DocumentObject*> groundedObjs;
    for (auto joint : groundedJoints) {
        App::DocumentObject* gndObj = AssemblyObject::getLinkObjFromProp(joint, "ObjectToGround");
        if (gndObj) {
            groundedObjs.push_back(gndObj);
        }
    }

    std::vector<App::DocumentObject*> childrens = assemblyPart->getAllChildren();
    for (auto child : childrens) {
        bool grounded = false;
        for (auto gndObj : groundedObjs) {
            if (child->getFullName() == gndObj->getFullName()) {
                grounded = true;
                break;
            }
        }

        if (grounded) {
            // add lock icon to child vp
        }
        else {
            // remove lock icon to child vp
        }

    }
}

Thoughts?

@PaddleStroke
Copy link
Contributor

Having said that there is already a lot of overlayed icons (recompute mark, external, link) so adding one more is maybe not so great in terms of visibility.

@tobiasfalk
Copy link
Contributor Author

@wwmayer is there a way to overlay an icon on top of any object icon? So that the lock icon can be overlayed on top of other icons :
image

Yes that's pretty much what I meant, but why not get the Icon, than overly the Lock and than put this in the TreeView.

@pierreporte
Copy link

pierreporte commented Feb 26, 2024

Solidworks displays the grounded status with text at the beginning of the tree-view text:

  • (f) for fixed parts,
  • (-) for non-fixed parts.

This text is before the formatted string explained in #12141 (comment).

Basically it’s as if there was an icon but more discreet.

Also considering #12141, the text could be quite long so the symbol should be at the beginning not at the end.

A small lock over the icon would be hard to see.

image

@wwmayer
Copy link
Contributor

wwmayer commented Feb 27, 2024

is there a way to overlay an icon on top of any object icon?

Try BitmapFactory().merge()

@tobiasfalk
Copy link
Contributor Author

Solidworks displays the grounded status with text at the beginning of the tree-view text:

  • (f) for fixed parts,
  • (-) for non-fixed parts.

...

Also sounds like a good solution, this with a modification to the Auto naming code so that it is ignored and it should work perfectly.
I would also add a (j) fo parts that are joined in a way so that one can see witch parts are completely free.

@maxwxyz maxwxyz changed the title [Problem] Automatic naming is confused/wrong with the grounded Symbol of the Assambly Workbench [Problem] Automatic naming is confused/wrong with the grounded Symbol of the Assembly Workbench Feb 27, 2024
@furgo16
Copy link
Contributor

furgo16 commented Mar 6, 2024

Also note that the lock as a character/pictogram (as opposed to an icon), might render differently on different platforms and/or different fonts. On Linux for instance, it is not even recognisable as a lock and it adds extra vertical padding to the tree. See the screenshot in the (related?) issue #12788

The lock in the label might also be the cause for another bug: #12789

In another note, thanks so much @maxwxyz for your great work triaging issues, really appreciated! As is the work of everyone contributing to the new integrated assembly workbench, which is awesome. Thanks!

@obelisk79
Copy link
Contributor

I like the lock symbol here. But not overlayed like the tip indicator. There much be a way to align it to the right of the label with it modifying the label itself. The behavior currently is inconsistent and will generate lots of unwanted feedback due to the unexpected behavior.

@PaddleStroke
Copy link
Contributor

PaddleStroke commented Mar 15, 2024

@wwmayer I'm still not sure how to handle this problem could you please weight in?
The thing is that to indicate in the tree that an object is grounded, we need (I think) to store this information in the ViewProviderDocumentObject.
Currently it is done so by adding the lock character to the object label. But this has many issues so we can't keep this.

Is it acceptable to add a 'Locked' bool property to ViewProviderDocumentObject ?

Then my idea was that if Locked is true, then either :

  • Overlay a lock icon on the existing icon. Pro : easy and don't clutter more the tree. Cons: Many overlays already: link, external, recompute, tip... Though not many would interfer
  • Add an icon next to main icon. Pro : clearer. cons: starts cluttering the tree with icons.
  • Add an icon to the right side of the label (where currently the character-icon appears). Pro : best solution imo. Cons : hard because how do we draw icon on the right side?

Thoughts?

@PaddleStroke
Copy link
Contributor

Another possible solution is that we do not show anything in the tree. Instead we draw a 'ground' in the 3d view :
image

This is perhaps the easier and less intrusive thing. Thoughts?

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 15, 2024

@PaddleStroke I don't like the ground. What about a small lock icon at the center of mass in the 3D view? I think in general it would be god to display the joint type alongside the JCS in the 3D view as it gets crowded when multiple joints are visible.

@PaddleStroke
Copy link
Contributor

Lock symbol why not, but if it's draw in 2d then when you rotate it's not ideal. Ideally it should rotate with the camera. Like in sketcher. But I'm not sure how this is implemented.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 15, 2024

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 15, 2024

@PaddleStroke
There is a fixed placed annotation:

obj=App.ActiveDocument.addObject("App::Annotation","Label")
obj.LabelText="Annotation"

or a movable Annotation label:

obj2=App.ActiveDocument.addObject("App::AnnotationLabel","Label")
obj2.LabelText="Annotation colour test"
annotations.mp4

Data and View Tab in the property editor allows for styling. This would be nice to see all joint locations and types!

@PaddleStroke
Copy link
Contributor

PaddleStroke commented Mar 15, 2024

image
So I killed the lock icon in the label and replaced it with a lock symbol on the 3d view like so.

Note: it does not remove locks from existing labels.

@wwmayer
Copy link
Contributor

wwmayer commented Mar 15, 2024

Is it acceptable to add a 'Locked' bool property to ViewProviderDocumentObject ?

I am not a fan of adding stuff to a base class that is only needed in a sub-class. It would be acceptable for me to extend the ViewStatus enum from ViewProvider.h because this at least doesn't require more memory.

@maxwxyz
Copy link
Collaborator

maxwxyz commented Mar 15, 2024

So I killed the lock icon in the label and replaced it with a lock symbol on the 3d view like so.

Looks good. Is it always visible, or only when the joint is visible? It could also just become visible if you try to drag this item or have the create/edit joint dialog open so you see which one is the grounded part(s).

@PaddleStroke
Copy link
Contributor

only when the joint is visible.
Yes become visible when trying to drag the item would be ideal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This issue or PR is related to a bug WB Assembly Related to the Integrated Assembly Workbench
Projects
None yet
7 participants