[Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

About the development of the Part Design module/workbench. PLEASE DO NOT POST HELP REQUESTS HERE!
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

[Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by jnxd »

Discussion thread for issue #7442.

What's happening is that individual points are considered to be construction, and we somehow decided not to allow driving constraints on construction geometries.

@abdullah, @paddle what do you say we just remove these condition from SketchObject::toggleDriving?

Code: Select all

    bool extorconstructionpoint1 =  (vals[ConstrId]->First == GeoEnum::GeoUndef) ||
                                    (vals[ConstrId]->First < 0)                     ||
                                    (geof1 && geof1->isGeoType(Part::GeomPoint::getClassTypeId()) && geof1->getConstruction()); // remove this condition
    bool extorconstructionpoint2 =  (vals[ConstrId]->Second == GeoEnum::GeoUndef)||
                                    (vals[ConstrId]->Second < 0)                    ||
                                    (geof2 && geof2->isGeoType(Part::GeomPoint::getClassTypeId()) && geof2->getConstruction()); // remove this condition
    bool extorconstructionpoint3 =  (vals[ConstrId]->Third == GeoEnum::GeoUndef) ||
                                    (vals[ConstrId]->Third < 0)                     ||
                                    (geof3 && geof3->isGeoType(Part::GeomPoint::getClassTypeId()) && geof3->getConstruction()); // remove this condition

    if (extorconstructionpoint1 && extorconstructionpoint2 && extorconstructionpoint3 && !vals[ConstrId]->isDriving)
        return -4;
My latest (or last) project: B-spline Construction Project.
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by paddle »

Somehow I can't replicate that issue on fc 0.20 win10 (neither on my current 0.21 dev branch).
Can you tell me precisely what to do?
Here's what I try :
- Make ellipse.
- toggle driving constraints
- create driven distance constraint between one focus point and center
- double click the blue constraint.
- uncheck 'reference'
- constraint turns driving as expected.

I also tried selecting the constrain then activating the toggle tool and it works too.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by openBrain »

What exactly means "internal geometries" ? Please provide an example file to replicate. ;)

Notice that for few months now single points can be toggled between normal and construction geometry.
chrisb
Veteran
Posts: 53919
Joined: Tue Mar 17, 2015 9:14 am

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by chrisb »

To reproduce the issue:
- edit the sketch
- select the constraint
- apply Sketcher ToggleDrivingConstraint
-> The constraint remains driven

Double clicking and changing to driving works as paddle described.

The issue doesn't occur on the endpoints of major and minor diameter.
Attachments
SnipScreenshot-cc91b4.png
SnipScreenshot-cc91b4.png (14.18 KiB) Viewed 5989 times
drivenEllipse.FCStd
(5.43 KiB) Downloaded 40 times
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by openBrain »

OK. Actually there is inconsistency. This is due to same "function" being coded both in 'SketchObject::toggleDriving' and 'SketchObject:setDriving' with slight differences.
Also 'Sketch::toggleDriving' implements some validity checks while there is already the 'SketchObject::testDrivingChange' function for this.

To me, the correct solution is to delete 'toggleDriving' code and just replace with something like :

Code: Select all

return setDriving(ConstrId, !Constraints.getValues()[ConstrId]->isDriving)
At least this brings consistency. Then we can eventually fine-tune validity checks in the dedicated function. ;)
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by jnxd »

OK seems I missed quite a bit here.

@paddle sorry for the confusion. I didn't notice that was another way to toggle.
@chrisb thanks for describing the issue better.
openBrain wrote: Mon Sep 05, 2022 10:07 am To me, the correct solution is to delete 'toggleDriving' code and just replace with something like :

Code: Select all

return setDriving(ConstrId, !Constraints.getValues()[ConstrId]->isDriving)
At least this brings consistency. Then we can eventually fine-tune validity checks in the dedicated function. ;)
@openBrain indeed since setDriving exists toggleDriving shouldn't be an independent function. However, I notice that setDriving has no validity checks. So it might be being used for other purposes.

I also missed mentioning here that I made PR #7443 to solve the issue. Of course, it can be used with or instead of openBrain's solution.
My latest (or last) project: B-spline Construction Project.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by openBrain »

jnxd wrote: Mon Sep 05, 2022 5:06 pm @openBrain indeed since setDriving exists toggleDriving shouldn't be an independent function. However, I notice that setDriving has no validity checks. So it might be being used for other purposes.
Of course it already has. As I said above, it calls 'testDrivingChange' which (only) purpose is the validity check. ;)
I also missed mentioning here that I made PR #7443 to solve the issue. Of course, it can be used with or instead of openBrain's solution.
My proposal was a whole solution, not an option to something else. ;)
The best solution IMO, but it's all personal and I may fail. :D
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by jnxd »

openBrain wrote: Mon Sep 05, 2022 10:07 am Also 'Sketch::toggleDriving' implements some validity checks while there is already the 'SketchObject::testDrivingChange' function for this.
This is possible. However, one particular error code "-4" does not exist here. The only additional check for that though is about GeoUndef or construction (see quoted code in OP). So we should first be sure that both those checks are unnecessary or can be safely added to setDriving also. Arguably GeoUndef is also < 0 (the check for "return -3") so without construction check it should be deletable.
openBrain wrote: Mon Sep 05, 2022 5:13 pm My proposal was a whole solution, not an option to something else. ;)
The best solution IMO, but it's all personal and I may fail. :D
There is room for both solutions: move the "return -4" check to SketchObject::testDrivingChange.
My latest (or last) project: B-spline Construction Project.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by openBrain »

jnxd wrote: Mon Sep 05, 2022 5:26 pm This is possible. However, one particular error code "-4" does not exist here.
IIRC, return code isn't used anywhere. Just have to return 0 for success, something else for failure. ;)
So we should first be sure that both those checks are unnecessary or can be safely added to setDriving also.
My guess ATM is that if these checks were really needed, we would know it because they aren't implemented when you toggle driving state from the datum dialog. ;) And anyway, dev branch is for dev.
There is room for both solutions: move the "return -4" check to SketchObject::testDrivingChange.
Exactly what I was talking about in my first post (last sentence). But you can call it "both solutions" if you want. :P
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: [Sketcher] Issue #7442: Driven constraints on internal geometries cannot be made driving

Post by jnxd »

@openBrain

In github you said,
My changes proposal would totally erase the current one, so adding another commit seems weird to me.
Hope the latest force push clears things a bit. If they are all OK, they can be squashed in the end, or we can remove the unneeded parts.
My latest (or last) project: B-spline Construction Project.
Post Reply