Dissallowing cross coordinate-system links

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
chrisb
Posts: 22428
Joined: Tue Mar 17, 2015 9:14 am

Re: Dissallowing cross coordinate-system links

Postby chrisb » Sun Aug 13, 2017 11:33 pm

peterl94 wrote:
Sun Aug 13, 2017 7:42 pm
The macOS builds are working again.
Yes they are, thanks.
peterl94
Posts: 1000
Joined: Thu May 23, 2013 7:31 pm
Location: United States

Re: Dissallowing cross coordinate-system links

Postby peterl94 » Sat Aug 19, 2017 2:44 am

Something should be done about the following GUI since neither of the highlighted options work when attempting to make a pad from a sketch in a different body.
pad-dialog.png
pad-dialog.png (13.92 KiB) Viewed 806 times
dialog.png
dialog.png (9.6 KiB) Viewed 806 times
Also, I noticed that disallowing cross coordinate system links removes some functionality that doesn't have an alternative yet. For example, you use to be able to do this:
body-fusion.png
body-fusion.png (14.28 KiB) Viewed 806 times
Do you have any plans for an alternate way to achieve that? It would be really useful to be able to do a boolean of two bodies with non-zero placement and then modify it with additional Part Design features.
ickby
Posts: 2940
Joined: Wed Oct 05, 2011 7:36 am

Re: Dissallowing cross coordinate-system links

Postby ickby » Sat Aug 19, 2017 5:16 am

Hello.

I already started with the task dialog you have shown. That will also pop up as normal dialog at.many places and does not work well there.

For the part design base object, yes, that is also known. But in general you can always use the parr design. Boolean. This has been refactored in the pull request, and now you can put in anything you want and placements should be considered correctly.. Other bodies, part objects etc... I think for the shown use case this is the better was.
ickby
Posts: 2940
Joined: Wed Oct 05, 2011 7:36 am

Re: Dissallowing cross coordinate-system links

Postby ickby » Sat Aug 19, 2017 6:41 am

By the way, I'm now in hollidays for more than a week, so no updates to be expected soon.
peterl94
Posts: 1000
Joined: Thu May 23, 2013 7:31 pm
Location: United States

Re: Dissallowing cross coordinate-system links

Postby peterl94 » Sat Aug 19, 2017 1:13 pm

ickby wrote:
Sat Aug 19, 2017 5:16 am
But in general you can always use the parr design. Boolean. This has been refactored in the pull request, and now you can put in anything you want and placements should be considered correctly.. Other bodies, part objects etc...
That sounds perfect, but I can't seem to get it to work. Please have a look at the attached document when you get back to it (created in the same version as previously posted).

Edit: Actually, the below document displays correctly. The problem I was having was that the boolean result wasn't shown after creating it, but I see now that it does after closing and reopening the doc.
Attachments
boolean-test.FCStd
(11.79 KiB) Downloaded 13 times
wmayer
Site Admin
Posts: 15483
Joined: Thu Feb 19, 2009 10:32 am

Re: Dissallowing cross coordinate-system links

Postby wmayer » Mon Aug 21, 2017 6:03 pm

In commit 180ff29c3f it's now also checked for isTouched() to determine if a feature must be recomputed. But doing so makes the various return values of mustExecute() pretty much useless and it removes the possibility of special feature classes to insist not to be executed even if touched.
So, is there a reason why this behaviour should change?

One of the unit tests fails. The stacktrace is:
Traceback (most recent call last):
File "E:\FClibs_vc12_x64\bin\lib\unittest\case.py", line 329, in run
testMethod()
File "C:\Projects\FreeCAD-git\ReleaseOCC\Mod\PartDesign\TestPartDesignGui.py", line 203, in testMoveSingleFeature
self.assertEqual(len(self.BodyTarget.Group), 2, "Target body feature count is wrong")
File "E:\FClibs_vc12_x64\bin\lib\unittest\case.py", line 513, in assertEqual
assertion_func(first, second, msg=msg)
File "E:\FClibs_vc12_x64\bin\lib\unittest\case.py", line 506, in _baseAssertEqual
raise self.failureException(msg)
AssertionError: Target body feature count is wrong
There are a couple of unit tests now that require user interaction. If possible it should be avoided to pop up any modal dialogs as this breaks any automatism.
User avatar
DeepSOIC
Posts: 7479
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Dissallowing cross coordinate-system links

Postby DeepSOIC » Mon Aug 21, 2017 6:20 pm

wmayer wrote:
Mon Aug 21, 2017 6:03 pm
In commit 180ff29c3f it's now also checked for isTouched() to determine if a feature must be recomputed. But doing so makes the various return values of mustExecute() pretty much useless and it removes the possibility of special feature classes to insist not to be executed even if touched.
So, is there a reason why this behaviour should change?
I want to have a reliable way to make the feature recompute upon next recompute. Fusion, for example, doesn't recompute if I call .touch() on it and click Recompute button. Which is a bit annoying.

As far as I understand, "touched" flag is supposed to mean "the object has been changed", and it doesn't imply "object must recompute itself". I find it a bit of a problem, because "touched" is used like a mix of "the object has been changed" and "object must execute", but not consistently, which makes it confusing. To force a recompute of a feature upon next doc.recompute, there should be a special flag, IMO.
wmayer
Site Admin
Posts: 15483
Joined: Thu Feb 19, 2009 10:32 am

Re: Dissallowing cross coordinate-system links

Postby wmayer » Mon Aug 21, 2017 7:17 pm

I want to have a reliable way to make the feature recompute upon next recompute. Fusion, for example, doesn't recompute if I call .touch() on it and click Recompute button. Which is a bit annoying.
I can't confirm this. Do you have an example?
As far as I understand, "touched" flag is supposed to mean "the object has been changed", and it doesn't imply "object must recompute itself".
In the first place it means that a property (which hasn't set the 'Prop_Output' flag) of an object has been which then also marks the object as touched.
I find it a bit of a problem, because "touched" is used like a mix of "the object has been changed" and "object must execute", but not consistently, which makes it confusing.
It's up to each feature class to implement its mustExecute() accordingly. Usually it checks all its properties and returns "1" if one of them is touched. Furthermore, it may or may not call the mustExecute() method of its base class or it can just check if the touched flag is set as it's done in the default implementation of DocumentObject.
User avatar
DeepSOIC
Posts: 7479
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Dissallowing cross coordinate-system links

Postby DeepSOIC » Mon Aug 21, 2017 8:06 pm

wmayer wrote:
Mon Aug 21, 2017 7:17 pm
I want to have a reliable way to make the feature recompute upon next recompute. Fusion, for example, doesn't recompute if I call .touch() on it and click Recompute button. Which is a bit annoying.
I can't confirm this. Do you have an example?
Sure!
1. Part Box.
2. Part Cylinder
3. Part Fuse the two.
4.

Code: Select all

App.ActiveDocument.Fusion.Shape = Part.makeSphere(5)
App.ActiveDocument.Fusion.touch()

5. Click Recompute. Fusion should restore its proper shape, but it doesn't.
wmayer
Site Admin
Posts: 15483
Joined: Thu Feb 19, 2009 10:32 am

Re: Dissallowing cross coordinate-system links

Postby wmayer » Tue Aug 22, 2017 11:59 pm

OK, I see but the correct way is to fix MultiFuse::mustExecute. Its implementation at the moment is

Code: Select all

short MultiFuse::mustExecute() const
{
    if (Shapes.isTouched())
        return 1;
    return 0;
}
but should be changed to

Code: Select all

short MultiFuse::mustExecute() const
{
    if (Shapes.isTouched())
        return 1;
    return Feature::mustExecute();
}
At the end Part::Feature::mustExecute() calls DocumentObject::mustExecute which already checks for the "touched" flag.

And this also fixes another problem which the supposed change in Document::recompute doesn't address:
with the integration of the extension concept by ickby an extension could be touched and this is also explicitly checked by DocumentObject

Code: Select all

short DocumentObject::mustExecute(void) const
{
    if(isTouched())
        return 1;

    //ask all extensions
    auto vector = getExtensionsDerivedFromType<App::DocumentObjectExtension>();
    for(auto ext : vector) {
        if(ext->extensionMustExecute())
            return 1;
    }
    return 0;
    
}