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!
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 »

abdullah wrote:#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?
Unfortunately, catching OCC exceptions will make freecad App depend on OCC. Currently it doesn't, all OCC stuff is delegated to Part module. As far as I understand, FreeCAD can be built without OCC if Path and all dependent modules are disabled.

PS. And stupid C++ exception handling doesn't offer a generic way of getting error message.
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:Unfortunately, catching OCC exceptions will make freecad App depend on OCC. Currently it doesn't, all OCC stuff is delegated to Part module. As far as I understand, FreeCAD can be built without OCC if Path and all dependent modules are disabled.

PS. And stupid C++ exception handling doesn't offer a generic way of getting error message.
Thanks! This is indeed a very good reason. :D

I am going to do a proposal for improvement for Base::Exception and derivatives.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

Ok. Before doing a exception extension proposal, I am going to PR what I have done to solve the problem that the user does not get informed of the wrong support and some clean up:

1. I have gone down all the Base:Exceptions that are thrown as a consequence of not being able to determine the position from the support, and converted them into derivatives.
2. I have changed the code to always propagate those Exceptions up in the hierarchy (to App::DocumentObject), so that the correct Status bits are set.
3. I have converted any OCC exception raised by this code into a Base::RuntimeError exception, and propagate it up to App:DocumentObject.

The effect is that now when changing the mapping, if the support is not appropriate for the mapping, the user sees the exclamation mark and a message indicating why when hovering the exclamation mark. The exception is also written to the report window.
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: PD NEXT: Mapping change and support

Post by wmayer »

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, originally it was shoogen. But it's better to use a specific when throwing an exception because then in the exception handlers you are able to handle some exception differently. If only using the same type everywhere this isn't be possible.
abdullah wrote:I am trying to figure the best mechanism for setting an error flag from attach extension.

This is the recompute stack
The actual problem is how DocumentObject::execute(void) is implemented. It calls the extensionExecute of its extensions but doesn't stop and forward the DocumentObjectExecReturn on errors. Furthermore it causes a memory leak because the result object gets never destroyed.
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?
A direct dependency of the core system on OCC is not desired. If we want some special OCC handling there is no problem to introduce a new exception class.
On feature level OCC exceptions are converted into a DocumentObjectExecReturn structure.

Code: Select all

App::DocumentObjectExecReturn *Feature::recompute(void)
{
    try {
        return App::GeoFeature::recompute();
    }
    catch (Standard_Failure) {
        Handle(Standard_Failure) e = Standard_Failure::Caught();
        App::DocumentObjectExecReturn* ret = new App::DocumentObjectExecReturn(e->GetMessageString());
        if (ret->Why.empty()) ret->Why = "Unknown OCC exception";
        return ret;
    }
}
The catch block is only reached when a feature's execute method doesn't handle OCC exceptions.
triplus
Veteran
Posts: 9471
Joined: Mon Dec 12, 2011 4:45 pm

Re: PD NEXT: Mapping change and support

Post by triplus »

abdullah wrote:The effect is that now when changing the mapping, if the support is not appropriate for the mapping, the user sees the exclamation mark and a message indicating why when hovering the exclamation mark. The exception is also written to the report window.
Sounds good.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: PD NEXT: Mapping change and support

Post by abdullah »

wmayer wrote:The actual problem is how DocumentObject::execute(void) is implemented. It calls the extensionExecute of its extensions but doesn't stop and forward the DocumentObjectExecReturn on errors. Furthermore it causes a memory leak because the result object gets never destroyed.
So, after the merge (thanks!), there is still a memory leak in the case of an OCC exception (see StandardFailure catch block):

https://github.com/FreeCAD/FreeCAD/pull ... 52df1a91f0

Is not it?

What is the meaning of DocumentObjectExecReturn? I see that it stores an object and a string from the declaration. I would appear that it wants to be a substitution mechanism for an exception. You gain knowing the object to which the string text is related. You lose knowing the type of exception.

Part of the question is related also to: Do the extensions need to handle DocumentObjectExecReturns? It is difficult for me to understand this without know the answer to the previous question, but it would appear that DocumentObjectExecReturns is the responsibility of the DocumentObject, a responsibility not (necessarily) to be delegated to the extensions. Shouldn't DocumentObject just catch whatever exception coming from the extensions and fill the structure as needed?
wmayer wrote:A direct dependency of the core system on OCC is not desired. If we want some special OCC handling there is no problem to introduce a new exception class.
On feature level OCC exceptions are converted into a DocumentObjectExecReturn structure.
I have seen that you have extended the number of exception classes in a separate commit. I welcome this very much.

RELATED TO EXCEPTIONS

Lately I have found some issues in the exception system that makes me less productive as a coder and generally come with a wrong or reduced information to the end user. I have been working into trying to improve the exceptions so that:

1. The exception includes a message intended to be shown to the user and potentially translated. This message does not contain any other information used upstream in the exception throwing and catching or for interaction with Python. There is no need to strip this string from prepended prefixes/suffixes. This message is usable directly for the Python Console AND the UI.

2. The exception includes a higher amount of information about in which function, file and line it was thrown. This has to be harvested automatically. This information is shown in the Report view and is mainly intended to allow developers to track complex workflows and improve the filling of bug reports.

This branch is to show what I have done so far (work in progress), so that you can give me your opinion:
https://github.com/abdullahtahiriyo/Fre ... provements

I do not mean it to be the solution. It may be the solution or a way to discuss it and gather further user/developer feedback in order to arrive to a better solution.

What I have done in words

1. I extended the exception class to hold separate data member fields for a translatable message, a prefix (probably deemed to be removed, as it seemed necessary for python interfacing, but now I think it won't be necessary), file and line fields for the exception.

2. I created macros to throw exceptions, so that function, file and line is automatically retrieved by the preprocesor/compiler and inserted into the exception. THROW to throw an exception without message, THROWM to throw an exception with a message but without a prefix, THROWMP to throw an exception with a message and a preffix.

The automatic harvesting of information is intended to avoid spoiling a translatable text message usable in the UI with function information and effectively automating it.

Classes that currently have a "what" with a mixture of translatable and non-translatable text (see FileDialogError exception, that has the string of the file name), still produce the same "what" for backwards compatibility, but include a separate data member to hold the translatable text portion.

How it behaves:

Practical example:
I modified, the sketcher modifyBsplineKnotMultiplicity to use the macros just for testing (do not try to give it meaning, it does not have one), like this:

Code: Select all

    if (GeoId < 0 || GeoId > getHighestCurveIndex())
        THROWM(Exception, "BSpline GeoId is out of bounds.")
        //throw Base::ValueError("BSpline GeoId is out of bounds.");
    
    if (multiplicityincr == 0) // no change in multiplicity
        THROWMP(Exception, "You are requesting no change in knot multiplicity.", "trial")
        
        //throw Base::ValueError("You are requesting no change in knot multiplicity.");
    
    const Part::Geometry *geo = getGeometry(GeoId);
    
    if(geo->getTypeId() != Part::GeomBSplineCurve::getClassTypeId())
        THROW(Exception)
        //throw Base::TypeError("The GeoId provided is not a B-spline curve");
This is how it behaves:

Python console:
App.ActiveDocument.Sketch004.modifyBSplineKnotMultiplicity(6,3,0)
Traceback (most recent call last):
File "<input>", line 1, in <module>
Base.FreeCADError: FreeCAD exception thrown (You are requesting no change in knot multiplicity.)

App.ActiveDocument.Sketch004.modifyBSplineKnotMultiplicity(16,3,0)
Traceback (most recent call last):
File "<input>", line 1, in <module>
Base.FreeCADError: FreeCAD exception thrown (BSpline GeoId is out of bounds.)

Report view:
Exception (Thu Apr 27 19:15:24 2017): BSpline GeoId is out of bounds. in bool Sketcher::SketchObject::modifyBSplineKnotMultiplicity(int, int, int) in src/Mod/Sketcher/App/SketchObject.cpp:4102

Exception (Thu Apr 27 18:58:34 2017): trial You are requesting no change in knot multiplicity. in bool Sketcher::SketchObject::modifyBSplineKnotMultiplicity(int, int, int) in src/Mod/Sketcher/App/SketchObject.cpp:4106

Exception (Thu Apr 27 19:25:07 2017): in bool Sketcher::SketchObject::modifyBSplineKnotMultiplicity(int, int, int) in src/Mod/Sketcher/App/SketchObject.cpp:4113

Where I am now?

Well I am taking a look into the Python part.

After quite a lot of digging and debuging, I have understood (or I think I have) this process of converting the c++ exception into Python exception and the throwing the c++ exception after python execution.

An example of this process:
1. We run a command, that gets executed via " presult = PyRun_String(sCmd, Py_file_input, dict, dict); /* eval direct */" in "std::string InterpreterSingleton::runString(const char *sCmd)"

2. *Py uses PyErr_SetString to associate a string to a python exception (could also use PyErr_SetObject, PyErr_Format). Example

Code: Select all

   catch(const Base::Exception& e) // catch the FreeCAD exceptions
    {
        std::string str;
        str += "FreeCAD exception thrown (";
        str += e.what();
        str += ")";
        e.ReportException();
        PyErr_SetString(Base::BaseExceptionFreeCADError,str.c_str());
        return NULL;
    }
3. The result in 1 is evaluated and if there is none, the python exception is evaluated:

Code: Select all

    if (!presult) {
        if (PyErr_ExceptionMatches(PyExc_SystemExit))
            throw SystemExitException();
        else
            throw PyException();
    }
4. This PyException constructor, gets the information for Python via:

Code: Select all

PyException::PyException(void)
{
    PP_Fetch_Error_Text();    /* fetch (and clear) exception */
    std::string prefix = PP_last_error_type; /* exception name text */
//  prefix += ": ";
    std::string error = PP_last_error_info;            /* exception data text */
5. Notably the exception information is obtained via PP_Fetch_Error_Text()

Code: Select all

void PP_Fetch_Error_Text()
{
    // called without exception happened!
    //assert(PyErr_Occurred());

    char *tempstr;
    PyObject *errobj, *errdata, *errtraceback, *pystring;

    /* get latest python exception information */
    /* this also clears the current exception  */

    PyErr_Fetch(&errobj, &errdata, &errtraceback);       /* all 3 incref'd */


    /* convert type and data to strings */
    /* calls str() on both to stringify */

    pystring = NULL;
    if (errobj != NULL &&
       (pystring = PyObject_Str(errobj)) != NULL &&      /* str(errobj) */
       (PyString_Check(pystring)) )                      /* str() increfs */
    {
        strncpy(PP_last_error_type, PyString_AsString(pystring), MAX); /*Py->C*/
        PP_last_error_type[MAX-1] = '\0';
    }
    else 
        strcpy(PP_last_error_type, "<unknown exception type>");
    Py_XDECREF(pystring);


    pystring = NULL;
    if (errdata != NULL &&
       (pystring = PyObject_Str(errdata)) != NULL &&     /* str(): increfs */
       (PyString_Check(pystring)) )
    {
        strncpy(PP_last_error_info, PyString_AsString(pystring), MAX); /*Py->C*/
        PP_last_error_info[MAX-1] = '\0';
    }
    else 
        strcpy(PP_last_error_info, "<unknown exception data>");
    Py_XDECREF(pystring);


    /* convert traceback to string */ 
    /* print text to a StringIO.StringIO() internal file object, then */
    /* fetch by calling object's .getvalue() method (see lib manual); */

    pystring = NULL;
    if (errtraceback != NULL &&
       (PP_Run_Function("StringIO", "StringIO", "O", &pystring, "()") == 0) &&
       (PyTraceBack_Print(errtraceback, pystring) == 0) &&
       (PP_Run_Method(pystring, "getvalue", "s", &tempstr, "()") == 0) )
    {
        strncpy(PP_last_error_trace, tempstr, MAX); 
        PP_last_error_trace[MAX-1] = '\0';
        free(tempstr);  /* it's a strdup */
    }
    else 
        strcpy(PP_last_error_trace, "<unknown exception traceback>"); 
    Py_XDECREF(pystring);


    Py_XDECREF(errobj);
    Py_XDECREF(errdata);               /* this function owns all 3 objects */
    Py_XDECREF(PP_last_traceback);     /* they've been NULL'd out in Python */ 
    PP_last_traceback = errtraceback;  /* save/export raw traceback object */
}
6. From this, the type of exception is obtained directly from PyErr_Fetch and without taking into account the prepended string at all.

I have also noted that in different workbenches, different friends of SetString are used (at least Format in PD).

I see that there is the possibility of setting an error indicator as SetObject (another friend of SetString). Taking into consideration the changes I mention above to the c++ exception classes, wouldn't it make sense to create an Object to hold all the exception information separately and avoid to try to code everything in a string? wouldn't it make sense to extend the types of python exceptions to have a one-to-one match with the c++ ones?

Sorry for the long post. If it looks like I am disoriented, then it is certainly because I am. Too many new things outside my Sketcher little parcel ;)
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 »

abdullah wrote:wouldn't it make sense to create an Object to hold all the exception information separately and avoid to try to code everything in a string? wouldn't it make sense to extend the types of python exceptions to have a one-to-one match with the c++ ones?
This is what I did in container exceptions: made a python exception type for every c++ exception. I'd be glad to see a system like this globally in FreeCAD. I suggest you start a new thread about this.
see: https://github.com/DeepSOIC/FreeCAD-ell ... ceptions.h
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: PD NEXT: Mapping change and support

Post by wmayer »

What is the meaning of DocumentObjectExecReturn? I see that it stores an object and a string from the declaration. I would appear that it wants to be a substitution mechanism for an exception.
Yes, that's what is supposed to do because it's a much less expensive way to handle errors. See also: http://preshing.com/20110807/the-cost-o ... -handling/

On the other hand, since inside most execute() exception handling is done anyway it almost renders useless to have the DocumentObjectExecReturn structure.
Shouldn't DocumentObject just catch whatever exception coming from the extensions and fill the structure as needed?
The way how it's implemented now doesn't make much sense. Either the DocumentObject::execute() should return as soon as an extension returns an error, or the return type of extensionExecute() should be void.
I have seen that you have extended the number of exception classes in a separate commit. I welcome this very much.
The plan is to make protected the constructors of the Base::Exception class. Then everyone is forced to use a suitable sub-class.
1. The exception includes a message intended to be shown to the user and potentially translated. This message does not contain any other information used upstream in the exception throwing and catching or for interaction with Python. There is no need to strip this string from prepended prefixes/suffixes. This message is usable directly for the Python Console AND the UI.
Translating messages of raised exceptions is not easy to realize. E.g. many exceptions are coming from 3rd party libs where you don't know what text is included and thus impossible to prepare for translations. Especially, OCC exceptions are very cryptic or even empty.

Besides this IMO we shouldn't even try to translate technical exceptions because this complicates the process of fixing errors. When translated messages are reported without further information you always have to go inside the translation files and search for the original text to find the location in the source code. And things become even worse if a user has an older version while the translation has been changed in the meantime.

And is there any benefit for a user to have translated exceptions? He may better understand what happened but he anyway won't be able fix it and has to report it to the developers.
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:This is what I did in container exceptions: made a python exception type for every c++ exception. I'd be glad to see a system like this globally in FreeCAD. I suggest you start a new thread about this.
see: https://github.com/DeepSOIC/FreeCAD-ell ... ceptions.h
I saw your other thread yesterday. I still have to digest it, but it seems to be an implementation of what I am missing as of today.
wmayer wrote:Yes, that's what is supposed to do because it's a much less expensive way to handle errors. See also: http://preshing.com/20110807/the-cost-o ... -handling/

On the other hand, since inside most execute() exception handling is done anyway it almost renders useless to have the DocumentObjectExecReturn structure.
wmayer wrote:The way how it's implemented now doesn't make much sense. Either the DocumentObject::execute() should return as soon as an extension returns an error, or the return type of extensionExecute() should be void.
Interesting. I will keep this in mind when revisiting the code.
wmayer wrote:The plan is to make protected the constructors of the Base::Exception class. Then everyone is forced to use a suitable sub-class.
... and create a new one if needed. I think is brilliant. it will also force to revisit every single Base::Exception currently in the project to use an appropriate derivative. :)
wmayer wrote:Translating messages of raised exceptions is not easy to realize. E.g. many exceptions are coming from 3rd party libs where you don't know what text is included and thus impossible to prepare for translations. Especially, OCC exceptions are very cryptic or even empty.

Besides this IMO we shouldn't even try to translate technical exceptions because this complicates the process of fixing errors. When translated messages are reported without further information you always have to go inside the translation files and search for the original text to find the location in the source code. And things become even worse if a user has an older version while the translation has been changed in the meantime.

And is there any benefit for a user to have translated exceptions? He may better understand what happened but he anyway won't be able fix it and has to report it to the developers.
No, there is no point in translating every exception. No, there is no point in trying to translate any exception text not originated by FreeCAD.

Generally speaking: It is a great disadvantage and pointless to try to translate every exception (I subscribe to your arguments). I think there is a big advantage from being able to reuse the text of selected exceptions for Console and UI.

Although it might become polemic, IMO we should not translate the App part, we should stick to translating the Gui part (I believe this is what we do now, although I do not know the whole project). This means that whatever comes in the python console or in the report view should not be translated. Here it comes the advantage to everybody from not translating. In this case, providing the function, file and line, I think it is a substantial improvement. This should allow better handling of the issues, not only by those who code, but also those who assist and manage the bug tracker. For the bug tracker I think we should always attach the console and report view text when reporting (UI messages many times are like and it says something like this when I translate from my language, many times we get lost in translation).

However, it is a substantial effort to prepare exception originating messages for the UI to show in a pop-up to the user. The thing is that in exceptions thrown by the FreeCAD developer (not those from third parties, like OCC), it is generally quite clear for the developer what the problem is when coding it (you gave me the GeoId of something that is not a BSpline and are asking me to increase the knot multiplicity, your GeoId is out of bounds, common you have no attachment or your attachment is invalid). In those cases and in that moment, using a properly thought exception framework, we could:

a) include every information needed for the exception for debugging purposes (see previous paragraph),
b) provide the information in such a way that it can be reused for the UI, in which case, the strings shall be translated.

For the case of third party exceptions catch by the code, it is still possible to throw the technical cryptic information to the report view and still give upstream a translatable message (code generated as we do not know what exactly happened) for the user. For example, we have some "BRepFill failed" OCC exception. Today, the user sometimes receives a pop-up with this wonderful text. Using a specific exception, CADKernelError, the translatable message could just be "CAD kernel error" or "CAD kernel error. See report view".

Nevertheless, from your input I see that it might be necessary to flag exceptions as having a translatable message or not having it, so as to enable the UI engine to know whether it should process the message within the exception (through the translation engine) or it should just display a generic error, being the text unsuitable for being shown to the user. I am thinking:
a) a boolean data member, parameter to the constructor with default value false and having specialized macros to throw it with false or true.
b) make another exception class (with protected constructors ;) ), TranslatableException and derive from it all the exceptions that have appropriate translatable messages, so the developer throws a translatable exception derivative if the text is suitable for UI or a class not derived from translatable exception if the text is not suitable for UI. Then the UI can take advantage of the type checking mechanism to know whether to show the message or a generic error message.

b) is more elegant than a). However it could come to some duplication of exceptions (having a translatable version and a non-translatable one).

What do you think after reading this additional information?
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: PD NEXT: Mapping change and support

Post by wmayer »

Me wrote:The way how it's implemented now doesn't make much sense. Either the DocumentObject::execute() should return as soon as an extension returns an error, or the return type of extensionExecute() should be void.
abdullah wrote:Interesting. I will keep this in mind when revisiting the code.
Just to be clear I am talking about this method:

Code: Select all

DocumentObjectExecReturn *DocumentObject::execute(void)
{
    //call all extensions
    auto vector = getExtensionsDerivedFromType<App::DocumentObjectExtension>();
    for(auto ext : vector)
        ext->extensionExecute();

    return StdReturn;
}
As said before it suppresses errors created by extensions and causes a memory leak. It better should look like this:

Code: Select all

DocumentObjectExecReturn *DocumentObject::execute(void)
{
    //call all extensions
    auto vector = getExtensionsDerivedFromType<App::DocumentObjectExtension>();
    for(auto ext : vector) {
        auto ret = ext->extensionExecute();
        if (ret != StdReturn)
            return ret;
    }

    return StdReturn;
}
No, there is no point in translating every exception. No, there is no point in trying to translate any exception text not originated by FreeCAD.
OK.
In this case, providing the function, file and line, I think it is a substantial improvement. This should allow better handling of the issues, not only by those who code, but also those who assist and manage the bug tracker.
I see.
For the case of third party exceptions catch by the code, it is still possible to throw the technical cryptic information to the report view and still give upstream a translatable message (code generated as we do not know what exactly happened) for the user. For example, we have some "BRepFill failed" OCC exception. Today, the user sometimes receives a pop-up with this wonderful text. Using a specific exception, CADKernelError, the translatable message could just be "CAD kernel error" or "CAD kernel error. See report view".
I agree.
a) a boolean data member, parameter to the constructor with default value false and having specialized macros to throw it with false or true.
I don't think we need this because a boolean alone doesn't help to mark text for translation.
b) make another exception class (with protected constructors ;) ), TranslatableException and derive from it all the exceptions that have appropriate translatable messages, so the developer throws a translatable exception derivative if the text is suitable for UI or a class not derived from translatable exception if the text is not suitable for UI. Then the UI can take advantage of the type checking mechanism to know whether to show the message or a generic error message.
I would prefer a special Exception class. Not sure if we need a whole set of classes for this or if one is sufficient.
b) is more elegant than a). However it could come to some duplication of exceptions (having a translatable version and a non-translatable one).
As said above we don't need a translated and an untranslated version for each exception type. When you have a look at Exception.h then most of them are used to describe some pure technical problems. The only type which may also show informative text to the user is RuntimeError. So, for me it's fine to have only one exception class used for translation.
Post Reply