Dissallowing cross coordinate-system links
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
Dissallowing cross coordinate-system links
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?
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?
Re: Dissallowing cross coordinate-system links
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.
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.
Re: Dissallowing cross coordinate-system links
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.ickby wrote:Are there any rejection to the idea itself
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
Need help? Feel free to ask, but please read the guidelines first
Re: Dissallowing cross coordinate-system links
Can you give some examples, as this statement is very general?ickby wrote: to not allow document objects link into other coordinate systems
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
Re: Dissallowing cross coordinate-system links
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.
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.
Re: Dissallowing cross coordinate-system links
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.Are there any rejection to the idea itself, anything that is not good code wise?
- DeepSOIC
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: Dissallowing cross coordinate-system links
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
- Veteran
- Posts: 7896
- Joined: Fri Aug 29, 2014 12:45 am
- Location: used to be Saint-Petersburg, Russia
Re: Dissallowing cross coordinate-system links
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 ), 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.
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 ), 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.
Re: Dissallowing cross coordinate-system links
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.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.
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.
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.I think I foresee a potential for crashing.
Re: Dissallowing cross coordinate-system links
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).