sketch Validation tool bug

[easyw-fc]

the missing coincidence detection is part of constrainator:
ActiveSketch.detectMissingPointOnPointConstraints

The open vertex is not there yet.
issue #3693

THX again for taking care of the Sketcher

[abdullah]

...

I leave you what I have come with in:
https://github.com/abdullahtahiriyo/Fre ... mmits/3690

If you open the sketch "sk2" in here with it:
https://forum.freecadweb.org/viewtopic. ... 12#p268645

you are notified in the error view of where the problem arises and the user gets a pop-up with a warning, like this:

Screenshot_20181116_180813.png (39.18 KiB) Viewed 1010 times

I have problems with some information in the report view due to my ignorance:
https://forum.freecadweb.org/viewtopic. ... 90#p269245

The report view looks like this now:

Code: Select all

Exception (Fri Nov 16 18:07:44 2018): Both points are equal In virtual void Part::GeomLineSegment::Restore(Base::XMLReader&) in src/Mod/Part/App/Geometry.cpp:3740 
Exception (Fri Nov 16 18:07:44 2018): Both points are equal In virtual void Part::GeomLineSegment::Restore(Base::XMLReader&) in src/Mod/Part/App/Geometry.cpp:3740 
Exception (Fri Nov 16 18:07:44 2018): FreeCAD Exception In virtual void Part::PropertyGeometryList::Restore(Base::XMLReader&) in src/Mod/Part/App/PropertyGeometryList.cpp:218 
Property -0.000000000000 of type G was subject to a partial restore.
Exception (Fri Nov 16 18:07:44 2018): FreeCAD Exception In virtual void App::PropertyContainer::Restore(Base::XMLReader&) in src/App/PropertyContainer.cpp:323 
Object "Sketch" was subject to a partial restore. As a result geometry may have changed or be incomplete.Exception (Fri Nov 16 18:07:44 2018): FreeCAD Exception In virtual void App::Document::Restore(Base::XMLReader&) in src/App/Document.cpp:1441 

In "readObjects()" of Document.cpp, I had to resort to a status bit, which is not cleared at this time. Code is a proof of concept.

All the problems of my previous post remain. It is something like: "hey maybe this functionality is nice to have", but the shoes are too big for me.

I would appreciate your point of view.

[wmayer]

The way how to handle all this goes into the right direction. However, there are some issues on the implementation that could be improved. Look at my comments on github, please.

[abdullah]

The way how to handle all this goes into the right direction. However, there are some issues on the implementation that could be improved. Look at my comments on github, please.

I am going through them. You are simply brilliant. Thanks!

[abdullah]

The way how to handle all this goes into the right direction. However, there are some issues on the implementation that could be improved.

Ok. I have re-implemented it centering on extending the XMLReader.

What I did:
1. Dump the exception mechanism altogether.
2. Extend XMLReader so as to "store" partial restore errors and propagate them upstream. How?
2a. By adding a StatusBits field with bits

Code: Select all

    enum ReaderStatus {
        PartialRestore = 0,                     // This bit indicates that a partial restore took place somewhere in this Document
        PartialRestoreInDocumentObject = 1,     // This bit is local to the DocumentObject being read indicating a partial restore therein
        PartialRestoreInProperty = 2,           // Local to the Property
        PartialRestoreInObject = 3              // Local to the object partially restored itself
    };

2b. Isn't one bit enough? Yes, for notifying to the topmost entity, but with the bits I can notify several instances up. This allows me to send valuable information to the report view that can help a user to see where the problem lies.
2c. The bits work in a hierarchical order:
2c.i) Once a partial restore problem arises ALL bits are set.
2c.ii) If the object that requested the restore, at some point is a PropertyList, the PartialRestoreInObject bit is cleared before every object is asked to be restored, checked and if problems arise notify to the report view about the type of object partially restored.
2c.iii) If a class requested the restore of a property (a property container), then before any property is asked to be restored the two lowest rank bits are cleared PartialRestoreInObject and PartialRestoreInProperty before the property is asked to be restored, and the PartialRestoreInProperty is checked, and if problems notify the report view about the Property partially restored.
2c.iv) If a class requested the restore of a DocumentObject, inside fails, the name of the Document object is sent to the Report view. Before every DocumentObject the three lowest rank bits are cleared and checked after the restore.
2c.v) the PartialRestore bit works at Document level and in this trial is never cleared.
3. The Document uses an own status bit to mark that there was a PartialRestore, which can be checked from the GUI and show a pop-up.

The code for review is here:
https://github.com/FreeCAD/FreeCAD/pull/1794

Is not intended for merge, but as a second proof of concept.

Output:

Screenshot_20181118_023349.png (150 KiB) Viewed 975 times

[wmayer]

Ok. I have re-implemented it centering on extending the XMLReader.

Great work!

Is not intended for merge, but as a second proof of concept.

Why not? The implementation looks already very good to me. There are only two or three improvements to do which I can do myself after merging the code.

Thanks again!

[abdullah]

Ok. I have re-implemented it centering on extending the XMLReader.

Great work!

Is not intended for merge, but as a second proof of concept.

Why not? The implementation looks already very good to me. There are only two or three improvements to do which I can do myself after merging the code.

Thanks again!

I am happy you liked it.

In the original idea, everything worked with exceptions. With the extension of the reader, no exceptions are needed to fix the specific case of the OP.

I have realised that you have added an exception catch block of RestoreError in your additions, I wonder if you have identified a situation where a exception is a better or the only alternative to directly setting the bits on the reader object, or if it is thought just as a way to offer more future flexibility.

[wmayer]

In the original idea, everything worked with exceptions. With the extension of the reader, no exceptions are needed to fix the specific case of the OP.

Yes, but there was too much forwarding of the exception to the calling instances. My actual idea was to raise the RestoreError only from a property that couldn't be restored any more. Then the PropertyContainer will catch the exception without forwarding it but only sets somewhere a flag that something went wrong. This flag then can be checked in the uppermost instance that started the project load and report it to the user.

I have realised that you have added an exception catch block of RestoreError in your additions, I wonder if you have identified a situation where a exception is a better or the only alternative to directly setting the bits on the reader object, or if it is thought just as a way to offer more future flexibility.

Yes, this is for more flexibility in the future as it's up to the author of a property class how he wants to handle errors on restoration (see above). Since we have introduced the new exception type RestoreError an author might use that instead of setting the flags.

[abdullah]

I have realised that you have added an exception catch block of RestoreError in your additions, I wonder if you have identified a situation where a exception is a better or the only alternative to directly setting the bits on the reader object, or if it is thought just as a way to offer more future flexibility.

Yes, this is for more flexibility in the future as it's up to the author of a property class how he wants to handle errors on restoration (see above). Since we have introduced the new exception type RestoreError an author might use that instead of setting the flags.

Good. Thanks for the insight.

[abdullah]

How does it sound?

a feature request if I can...
'cause you are working on the sketcher, would it be possible to expose the Validation tools (find missing coincidences and find open vertexes) to python?

Code: Select all

>>> ActiveSketch.OpenVertices
[(-105.0, 6.0, 0.0), (-105.0, 49.0, 0.0)]

PR:
https://github.com/FreeCAD/FreeCAD/pull/1798