App:Property in ViewProvider::updateDate issue?

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!
Post Reply
7ofNine
Posts: 27
Joined: Sat Aug 27, 2016 2:35 pm

App:Property in ViewProvider::updateDate issue?

Post by 7ofNine »

I didn't find anything quickly that this might have been discussed before.
History:
In the recent 18.1 official installation in windows when using the image workbench and trying to put an image on a plane in 3D space I end up with an exception. But before that happens I see in the report window "QFSFileEngine::open: No file name specified", a message that was not present in FreeCAD 17 and definitely indicates an error. Thanks to the new libpack I could set up a debug environment where the exception doesn't happen but that message is still present.

Observation:
Hunting it down down brought me to the following code that looks rather suspicious to me and definitely causes the error message (and possibly also the exception in the current version).

It happens in addObject in the call to ViewProviderImagePlane::updateData(const App::Property* prop) from ViewProviderDocumentObject::updateView(). In updateView a map of App::Property * is created (getPropertyMap) and the Property* component of the elements of this map are then, in a loop, fed into the updateData method. In the specific case I am looking at there are actually 6 Property* and precisely one of them is an ImagePlane *. The first statements of the updateData method are

Code: Select all

...
void ViewProviderImagePlane::updateData(const App::Property* prop)
{
    Image::ImagePlane* pcPlaneObj = static_cast<Image::ImagePlane*>(pcObject);
    if (prop == &pcPlaneObj->XSize || prop == &pcPlaneObj->YSize) {
        float x = pcPlaneObj->XSize.getValue();
        float y = pcPlaneObj->YSize.getValue();

        //pcCoords->point.setNum(4);
...
which looks rather suspicious to me. A static cast to ImagePlane* when only one of the incoming pointers is of this type? pcPlaneObj refers in all other cases to "something" and in one of them it actually manages to run into the branch where it tries to open a file with an empty name (another minor error, there is no check on empty file name). It is miraculous that it doesn't crash all the time.

I think that this is an error and it can create all kinds of havoc or does anybody know of a good reason why this should actually work properly?

BTW: There are a few more cases where a static_cast is used in the above way in the updateData method of some ViewProviders. If the case above is an error all of them might have to be fixed.

Cheers
Hartmuth
Last edited by 7ofNine on Fri May 10, 2019 10:30 pm, edited 2 times in total.
chrisb
Veteran
Posts: 53945
Joined: Tue Mar 17, 2015 9:14 am

Re: App:Property in ViewProvider::updateDate issue?

Post by chrisb »

A small formal hint: if you use code tags </>> for the program code it is easier to distinguish and easier to read due to keeping indentation. You can edit your post and change it.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
7ofNine
Posts: 27
Joined: Sat Aug 27, 2016 2:35 pm

Re: App:Property in ViewProvider::updateDate issue?

Post by 7ofNine »

There is a small correction to the above statements.

Actually none of the Property* is of type ImagePlane* . One of the Property*'s coming from the ImagePlane object is of type ImageFile*. It doesn't change the argument though. static_cast-ing anything to a type that it is not is, according to the standard, undefined behaviour. A dynamic cast would here always return NULL.
There is also a problem with putting a svg file on a plane. It is rendered rather badly, while just reading it in the image view looks just fine.
There is also the performance issue that the image file is actually read twice. Once in order to determine the size information and a second time when it is actually put onto the plane i.e. the appropriate ViewProvider class. This can probably avoided if the corresponding properties are populated when the file is read onto its ViewPlane (XSize, YSize.)
User avatar
wandererfan
Veteran
Posts: 6270
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: App:Property in ViewProvider::updateDate issue?

Post by wandererfan »

7ofNine wrote: Fri May 10, 2019 7:18 pm I think that this is an error and it can create all kinds of havoc or does anybody know of a good reason why this should actually work properly?
You are right, there should be a dynamic_cast and a test for nullptr before trying to use pcPlaneObj.

I think the only thing that saves this from causing more trouble is that the pcObject* is "almost certainly" an Image::ImagePlane*.

The parent class (ViewProviderDocumentObject) sets pcObject in the attach() method. If attach() for ViewProviderXXX is setting pcObject to point to FeatureYYY then things have already gone terribly wrong and are about to get worse.
7ofNine
Posts: 27
Joined: Sat Aug 27, 2016 2:35 pm

Re: App:Property in ViewProvider::updateDate issue?

Post by 7ofNine »

A dynamic_cast might work but I think using FreeCAD's own type system might be a better answer. I'll have to stare at it a little bit longer.
User avatar
wandererfan
Veteran
Posts: 6270
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: App:Property in ViewProvider::updateDate issue?

Post by wandererfan »

7ofNine wrote: Sat May 11, 2019 2:30 am A dynamic_cast might work but I think using FreeCAD's own type system might be a better answer. I'll have to stare at it a little bit longer.
There are two patterns commonly used within FC

Code: Select all

if myObjPointer->isDerivedFrom(myClass::getClassTypeId())
      myClass* myGoodPointer = static_cast<myClass*>(myObjPointer)
     //do some stuff
- or -
myClass* myGoodPointer = dynamic_cast<myClass*>(myObjPointer)
if myGoodPointer != nullptr
   //do some stuff
Both are used, sometimes in the same program. Doesn't seem to be any difference in practice.
7ofNine
Posts: 27
Joined: Sat Aug 27, 2016 2:35 pm

Re: App:Property in ViewProvider::updateDate issue?

Post by 7ofNine »

Thank's for the help. I also see now what I didn't see before (pun intended) which threw me off. I am not used to using address comparisons as a discriminator in a C++ program but that seems to be the FC style. Nonetheless there is that small bug with trying to open a file with no name. I am also wondering why the XSize and the YSize is being set externally through the python interface i.e. they are writable attributes, when they are determined by the actual data in the file. A scaling information for an svg file might be an exception from this, I have to think about it.
Thanks again.
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: App:Property in ViewProvider::updateDate issue?

Post by wmayer »

Actually using the static_cast instead of dynamic_cast is OK for the view provider classes.
The sub-classes of DocumentObject reimplement the method getViewProviderName() that returns the class name of its view providers. The view provider class then can rely on that the DocumentObject is of the expected type.

The programmers don't have to worry about the creation of DocumentObject instances and the view providers. It's all done by the document class.

There are very few view provider classes that can be used in a more general way. There it matters what type the DocumentObject is of and a type checking is a must.
Post Reply