Dissallowing cross coordinate-system links

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
ickby
Posts: 2418
Joined: Wed Oct 05, 2011 7:36 am

Dissallowing cross coordinate-system links

Postby ickby » Sat Feb 18, 2017 12:41 pm

Hello,

as discussed already in multiple forum thread we came to the conclusion that it makes sense to not allow document objects link into other coordinate systems. This way they don't need to know anything about coordinate systems and transformations etc. In my branch I now included some code that enforces this behavior. This enforcement is important, as otherwise all kind of mess can happen. However, it is also rather intrusive. Thats why I like to show it here first and put it on discussion.

The relevant changes:

- Links that create a cycle in the dependency graph are rejected
- Links to other coordinate systems are rejected
- New link properties for links to other coordinate systems for special features that need those links.

https://github.com/ickby/FreeCAD_sf_mas ... PartDesign

https://github.com/ickby/FreeCAD_sf_mas ... ks.cpp#L82

Are there any rejection to the idea itself, anything that is not good code wise?
triplus
Posts: 4845
Joined: Mon Dec 12, 2011 4:45 pm

Re: Dissallowing cross coordinate-system links

Postby triplus » Sat Feb 18, 2017 1:20 pm

I guess it's more or less up to you for now. We likely don't have such insight to know if it makes sense or not. That is from the external link policy (not code) point of view. I wouldn't mind if i could create such link to external geometry. But you say it makes your life easier if i can't do that. Knowing how much effort you are investing in making PartDesign NEXT in good shape it makes sense for you to focus on what you can do for now in straightforward fashion. If you are in doubt in other areas focus on the solutions that make your life easier for now. I am sure you have a good overview by now.

P.S. And it's not like this can't be changed in the future (after FreeCAD 0.17 is released) if that would make sense and somebody would add the feature.
cox
Posts: 912
Joined: Wed Nov 26, 2014 11:37 pm

Re: Dissallowing cross coordinate-system links

Postby cox » Sat Feb 18, 2017 1:44 pm

ickby wrote:Are there any rejection to the idea itself


I can not say that I have a understanding of the ramifications of this, but there is a small voice in the back of my mind that tells me that this might be limiting the unrestricted nature of FreeCAD. The other voice tells me that you have considered this and that the new link properties will remedy all of the practical implications. :-)

I am trying to understand the new workings of 0.17 but my insight is way to minuscule for my input to be considered in any way, so I would like you to consider this an insight into the mind of a spectator that does not understand what is going on.

Keep up the good work. Godspeed :D
Need help? Feel free to ask, but please read the guidelines first
ulrich1a
Posts: 1374
Joined: Sun Jul 07, 2013 12:08 pm

Re: Dissallowing cross coordinate-system links

Postby ulrich1a » Sat Feb 18, 2017 4:07 pm

ickby wrote: to not allow document objects link into other coordinate systems
Can you give some examples, as this statement is very general?
My questions:
Has a document object like a spreadsheet a coordinate system? Does it get one, if it links to a body?
Is this also valid for the TechDraw workbench, which , I assume, has its own coordinate system?
Is an expression also a document link?

Ulrich
ickby
Posts: 2418
Joined: Wed Oct 05, 2011 7:36 am

Re: Dissallowing cross coordinate-system links

Postby ickby » Sat Feb 18, 2017 4:37 pm

Sorry, my explanaition has been to sparse, I'm just a bit to deep in the stuff :)

A local coordinate system is represented by an GeoFeatureGroup. Everything that is a GeoFeatureGroup like a PartDesign::Body or a App::Part apply their placement to all their children. Everything that is within a GeoFeatureGroup lives relative to each other, just like FreeCAD is now, the objects share the same reference frame. However, once objects are in different GeoFeatureGroups they do not share the same reference frame anymore. But they don't know that. They can't anticipate the position of the other feature correctly. It is all about placements and how they correlate. This also means that for objects that are not Geometrical, everything that is not a GeoFeature, those problems don't arise.

1. Only direct links to GeoFeatures and GeoFeatureGroups are checked and handled
2. Expressions are not checked, they can still reference everything. This is a bit of a break of the principle when it comes to using placements of GeoFeatures from within an expression, but well, we need to accept that.
3. Spreadsheets are not GeoFeatures, they can be referenced from within a GeoFeatureGroup to everywhere

Regarding TechDraw I don't know how it is coded. I don't think it uses GeoFeatureGroups so it has no own coordinate system, but could of course be wrong. I think Wandererfan must think about how to handle Cross-Coordinate-System links for his workbench. It does for one make sense to allow a TechDraw sheet only for Parts in the same GeoFeatureGroup as the sheet, as then all Placement would be immediately clear (and moving the GeoFeatureGroup would not move the drawing). But this seems also a bit cumbersome and as an artifitial restriction for tech draw. Hence he could also port it to use the Cross-CS-Link properties and than make up his mind how to deal with the placement problems.
wmayer
Site Admin
Posts: 11222
Joined: Thu Feb 19, 2009 10:32 am

Re: Dissallowing cross coordinate-system links

Postby wmayer » Sat Feb 18, 2017 5:02 pm

Are there any rejection to the idea itself, anything that is not good code wise?

I had a quick look and the code looks good so far. I have seen that of some classes the property type has changed. So, take care that old project files from 0.16 or earlier can still be read.
DeepSOIC
Posts: 4466
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Dissallowing cross coordinate-system links

Postby DeepSOIC » Sat Feb 18, 2017 10:19 pm

Consider I want to write a custom command that will transfer a feature from one container to another, adjusting the links in the process. State in the beginning is valid. State in the end is valid. State during transfer can be terribly bad (with circular dependencies, with cross-cs links, with who knows what else). Can the new "law enforcement" system screw everything up, or there is a way to temporarily disable it? I'm sorry I haven't bothered digesting the code yet. But from your posts it feels like the enforcement system is real-time and deep within... which is something that doesn't sound all right to me.
DeepSOIC
Posts: 4466
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: Dissallowing cross coordinate-system links

Postby DeepSOIC » Sun Feb 19, 2017 1:36 am

I've looked at some of the code, and made some comments on GitHub.

I think I foresee a potential for crashing.
You use recursive gathering of dependencies in quite a few places, with no safeguards against infinite recursion except the "law enforcement" system that should prevent a non-DAG dependency graph in the first place. But this system is disabled in several situations:
* undo
* restore from file
While that is OK in general (one would want the file to be read out exactly as it is). But the file may somehow contain a non-dag graph (e.g. created in older FreeCAD), so calling some of these recursive things may cause an out-of-stack-space error (probably a crash). More so, some of these functions seem to be called whenever a link property is altered, which will effectively prevent one from fixing the document within FreeCAD, and force to do it by editing a file directly.

Same with undo. Sometimes, undo transactions are not properly recorded (coding error). If undo transactions are not properly laid out (e.g., Part-o-magic might be an obnoxious offender in this regard :oops: ), undoing something may result in a non-dag graph, because some other link change was not in a transaction and thus is not being undone. This can cause a crash.
ickby
Posts: 2418
Joined: Wed Oct 05, 2011 7:36 am

Re: Dissallowing cross coordinate-system links

Postby ickby » Sun Feb 19, 2017 7:58 am

DeepSOIC wrote:Consider I want to write a custom command that will transfer a feature from one container to another, adjusting the links in the process. State in the beginning is valid. State in the end is valid. State during transfer can be terribly bad (with circular dependencies, with cross-cs links, with who knows what else). Can the new "law enforcement" system screw everything up, or there is a way to temporarily disable it? I'm sorry I haven't bothered digesting the code yet. But from your posts it feels like the enforcement system is real-time and deep within... which is something that doesn't sound all right to me.

You are right, that is an issue. Normally one would process all objects that need to be handled, like remove all objects from a group at once, and than adding them all at once to annother group. For this there are new "addObjects" etc. functions which take lists.

In general I think making the check real time at link creation is important to ensure that it is done correctly. One could resort to do it only on recompute, but I personally think this is a bit too late. It is nice to have the feedback what works and what not directly at time of creation, this makes it easier due to the clear feedback.
But I see your point of messing things up before getting to a good state again. We could think about adding enabling/disabling the check to the document and do a full link analysis when the check is enabled again (for recompute it must be enabled then).

EDIT: Annother reason for the direct check is that other functions need the correct link structure to work. If you want to retrieve the group or geofeature group of a object it must be ensured that there is only one (and the graph is DAG). Hence not checking links at creation time makes many functions work undefined.

I think I foresee a potential for crashing.

Those are good points. One needs to disable the checks for those restore and transaction actions, as everything fails otherwise. But I could easily add a afterward check when restore and transaction is done.
triplus
Posts: 4845
Joined: Mon Dec 12, 2011 4:45 pm

Re: Dissallowing cross coordinate-system links

Postby triplus » Sun Feb 19, 2017 2:02 pm

Well the truth is if you will forget to prevent such link to be created in some scenario and i will find that way. And if it will in general work for me for some task. I won't report it as a bug (as i will considered it to be a feature). ;)