PD NEXT: Mapping change and support

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
triplus
Veteran
Posts: 9471
Joined: Mon Dec 12, 2011 4:45 pm

Re: PD NEXT: Mapping change and support

Post by triplus »

DeepSOIC wrote:
triplus wrote:
abdullah wrote:In any case, I deem unacceptable to silently ignore a wrong support. FreeCAD should do something.
Exclamation mark in the tree view suggesting feature has a wrong support?
It used to do that in past. It looks like it's broken.
@abdullah

I guess you have just found a bug!
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

DeepSOIC wrote:
abdullah wrote:Is there any way to call on the attachment editor for a sketch?
Yes. Switch to Part WB. Select the sketch, and invoke Part->Attachment from the menu.

I initially wanted to add it to Edit menu, to be available in all workbenches. However my change wasn't accepted because it was "spaghetti code" (FreeCAD itself requesting features from other module).
Is it useful in all other workbenches? Well, I guess it should be possible to implement in FreeCAD itself some kind of hook that when executed adds extra functionality offered by the modules to none specific menus. FreeCAD would not know anything about what it is done with it, only where to put it in the menus. The module would be responsible for doing checks so that it is executed only in objects of the correct type. Such a functionality has general character (it is useful for multiple inter-workbench cooperation), avoids code duplicity and I won't regard such a thing as "spaghetti code", but I am not the one having to regard anything ;)
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

DeepSOIC wrote:
abdullah wrote:Is there any way to call on the attachment editor for a sketch?
Yes. Switch to Part WB. Select the sketch, and invoke Part->Attachment from the menu.
BTW, Thanks!! I got concentrated in the second part of your answer. It is absolutely counterintuitive. We shall do something about it.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

DeepSOIC wrote:
triplus wrote:
abdullah wrote:In any case, I deem unacceptable to silently ignore a wrong support. FreeCAD should do something.
Exclamation mark in the tree view suggesting feature has a wrong support?
It used to do that in past. It looks like it's broken.
I will try to reestablish the exclamation mark and throw an error to the error console. I will also refactor the support validation code into a member function, so that it can be reused elsewhere.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

I am trying to figure the best mechanism for setting an error flag from attach extension.

This is the recompute stack:

Code: Select all

#0 Part::AttachExtension::extensionExecute() 
#1 App::DocumentObject::execute() 
#2 Part::Feature::execute()
#3 App::DocumentObject::recompute() 
#4 Part::Feature::recompute() 
#5 App::Document::_recomputeFeature() 
#6 App::Document::recompute() 
I have managed to make it work like this. In #0:

Code: Select all

           if(acceptableattachment) {
                positionBySupport();
            }
            else {
                throw Base::ValueError("Current support is invalid for the selected mapping mode");                
            }
        /*} catch (Base::Exception &e) {
            return new App::DocumentObjectExecReturn(e.what());*/
Note that I had to comment out the catch(Base:Exception).

Why?

I had to do that because:
1. The one ultimately controlling the setting of the flag (and that will clear it if not receiving what it expects) is: Document::_recomputeFeature(DocumentObject* Feat) #5
2. This function will reset any flag set unless: a) the return code is not DocumentObject::StdReturn or b) a Base:Exception was thrown during the execution of the Feature->recompute() #4
3. Feature->recompute() #4 will return whatever #3 returned
4. #3 will return whatever #2 returned
5. #2 will return whatever #1 returned
6. #1 always, unconditionally returns StdReturn
7. at #0, the commented Base::Exception would catch my Base::ValueError exception that is derived from Base::Exception, thereby removing any hope of propagating the error information to #5

Now somebody took extraordinary care to catch all possible exceptions in #0 and return an "App::DocumentObjectExecReturn" which is nevertheless ignored in #1.

And that is the ultimate reason why I am writing this. Is there any reason for this? Can I just propagate all Base::Exceptions as with the commented code?

I also can catch first a Base::ValueError to re-throw the same and avoid the catching by Base::Exception that may be needed for other exceptions, but in such a case I am not returning the "App::DocumentObjectExecReturn".

Sorry for the long post and thanks in advance.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

More to it. DeepSOIC was right that there was code for raising an error, that exception catching that I commented out, will make it propagate the error.

So at the end the question is whether we can just comment out that exception catching:

https://github.com/abdullahtahiriyo/Fre ... 52df1a91f0

I think this is an acceptable solution. The user gets notified that there is something wrong and how to solve it.

Please, let me know your impressions.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: PD NEXT: Mapping change and support

Post by DeepSOIC »

I think you should catch all OCC exceptions and re-throw them as Base::RuntimeError, then.
(wmayer seems to usually recommend against throwing base::exception, and to pick a more specific type)
User avatar
sgrogan
Veteran
Posts: 6499
Joined: Wed Oct 22, 2014 5:02 pm

Re: PD NEXT: Mapping change and support

Post by sgrogan »

abdullah wrote:I would welcome more user input. What does a user want here?
I like the exclamation mark. The more verbose/specific error thrown to report view the better. As a user I'm not sure what is a bug or user error w/0.17.
"fight the good fight"
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

DeepSOIC wrote:I think you should catch all OCC exceptions and re-throw them as Base::RuntimeError, then.
(wmayer seems to usually recommend against throwing base::exception, and to pick a more specific type)
Well, at first I was trying to make the lowest amount of changes, as I am not familiar with PD NEXT and I tend to think that things are there for a reason.

However, I think you are right in that there is no point in catching the OCC ones there and be silent. #5 currently catches:

Code: Select all

    catch(Base::AbortException &e){
        e.ReportException();
        _RecomputeLog.push_back(new DocumentObjectExecReturn("User abort",Feat));
        Feat->setError();
        return true;
    }
    catch (const Base::MemoryException& e) {
        Base::Console().Error("Memory exception in feature '%s' thrown: %s\n",Feat->getNameInDocument(),e.what());
        _RecomputeLog.push_back(new DocumentObjectExecReturn("Out of memory exception",Feat));
        Feat->setError();
        return true;
    }
    catch (Base::Exception &e) {
        e.ReportException();
        _RecomputeLog.push_back(new DocumentObjectExecReturn(e.what(),Feat));
        Feat->setError();
        return false;
    }
    catch (std::exception &e) {
        Base::Console().Warning("exception in Feature \"%s\" thrown: %s\n",Feat->getNameInDocument(),e.what());
        _RecomputeLog.push_back(new DocumentObjectExecReturn(e.what(),Feat));
        Feat->setError();
        return false;
    }
#ifndef FC_DEBUG
    catch (...) {
        Base::Console().Error("App::Document::_RecomputeFeature(): Unknown exception in Feature \"%s\" thrown\n",Feat->getNameInDocument());
        _RecomputeLog.push_back(new DocumentObjectExecReturn("Unknown exeption!"));
        Feat->setError();
        return true;
    }
#endif
I am not sure if I want to convert (catch-rethrow) the OCC ones to a Base::RuntimeError, as I will be losing information, i.e. that it comes from OCC. #5 already handles std:exception, why not including a specific catch for StandardFailure in #5? Or should we make a more specific derivative from Base::Exception, like for example, Base::CadKernelError?
wmayer wrote:...
Do you have any preference/requirement regarding this?
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

sgrogan wrote:
abdullah wrote:I would welcome more user input. What does a user want here?
I like the exclamation mark. The more verbose/specific error thrown to report view the better. As a user I'm not sure what is a bug or user error w/0.17.
I would also like to standardize the exception error messages, so that it is possible to remove the pre-tails and apply translations for the UI. In most cases, it does not make sense to write one text to the output console and then generate a fully new text for the UI translation.
Post Reply