View Issue Details

IDProjectCategoryView StatusLast Update
0002735SketcherBugpublic2017-06-21 20:34
ReporterMajorTeslaAssigned Toabdullah 
PriorityhighSeveritymajorReproducibilityalways
Status closedResolutionfixed 
PlatformOSWindowsOS Version10
Product Version0.16 
Target Version0.17Fixed in Version0.17 
Summary0002735: Incorrect angle remains on deletion
DescriptionWhen a line is deleted, the angle dimensioning changes but the value stays the same.
Steps To Reproduce1. Start FreeCAD
2. Create a new empty document.
3. Create a new sketch on the XY plane
4. Draw a large rectangle
5. Close Tasks
6. Make a Pad
7. Close Tasks.
8. Select the face, facing you.
9. Draw a line from the origin, up and to the right at about 70deg.
10. Draw a small circle at the top end of the line.
11. Select the line then the Y axis line.
12. Select the 'Fix the angle of line' tool and make the angle of about 15deg.
13. Select and move the angle dimension above the circle (easier to see).
14. Select the line and press the delete key.

The angle dimension will read the same value but will now extend over 90deg.
TagsNo tags attached.

Activities

jnxd

2016-10-13 04:18

developer   ~0007361

It appears it also causes some other artifacts like making any other circles added invisible until the fake dimension is moved.

jnxd

2016-10-19 05:34

developer  

bug_2735_replicate.FCMacro (780 bytes)

jnxd

2016-10-19 05:43

developer   ~0007394

Attached a file that could replicate the bug with minimal operations. Run "bug_2735_replicate.FCMacro" after opening a new/blank sketch. The angle constraint remains even after the line is deleted if the second constraint (coincident, line 10) is also applied, but without it the bug does not happen. Used the following version:

OS: Ubuntu 16.04.1 LTS
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.17.8676 (Git)
Build type: None
Branch: master
Hash: b8163a03dc80b64b38744ed1289e8a7bdab0d378
Python version: 2.7.12
Qt version: 4.8.7
Coin version: 4.0.0a
OCC version: 6.8.0.oce-0.17

wmayer

2016-10-19 17:05

administrator   ~0007398

The bug is in method SketchObject::delGeometry()
The sketch has three constraints at this point:
constraint 1: coincident
constraint 2: coincident
constraint 3: angle, the values of the indexes are: First=1 (the geo id of the line), Second=-2 (the geo id of the vertical axis), Third=-2000 (undefined)

In two iteration steps this method removes the coincident constraints inside delConstraintOnPoint(). After the first iteration the values of the angle constraint are still correct but with the second step First suddenly becomes -1.

Afterwards in a second loop all constraints are deleted if either First, Second, or Third has the same value as GeoId. Since for the angle constraint all values are different it won't be deleted.

jnxd

2016-10-20 04:27

developer   ~0007406

Last edited: 2016-10-20 04:28

View 2 revisions

@wmayer: In my case there are 2 constraints, First for the angle constraint is 0 in the beginning, and it seems to change during the call to transferConstraints(). It appears the origin is marked as the start point of the x-axis (GeoId=-1). The implementation raises a question of what does one mean by the FirstPos and SecondPos for an angle constraint, and is there a way to ensure the transferConstraints doesn't change angle constraints unless perhaps there is a line segment parallel to the deleted one.

wmayer

2016-10-23 16:29

administrator   ~0007417

It seems the logic of deletion of geometries and adjusting constraints is not well thought out. There are so many different kind of constraints which needs an explicit handling somewhere but the current implementation probably makes some implicit assumptions about GeoIds and PointPos values.

Here is another example that doesn't correctly clear all associated constraints:
1. draw a line
2. add a point-on-curve constraint with the line and the vertical axis
3. with the same point of the line and the root point make a coincidence
4. delete the line
=> on the left side the filter shows the point-on-curve constraint

This kind of problem comes up if you add constraints between the vertical axis and a geometry and a coincidence between a point of the geometry and the root node. Now internally the GeoId of the vertical axis is -2, -1 of the horizontal axis and also -1 of the root point. Due this ambiguity it happens that when deleting the geometry and with it the coincidence that the -1 of the root node gets interpreted as the horizontal axis.

wmayer

2016-10-23 16:46

administrator   ~0007418

> The implementation raises a question of what does one mean by the FirstPos and SecondPos for an angle constraint

@ jnxd: As far as I could figure it out the FirstPos and SecondPos values are used to decide where to draw the angle symbol between the two lines.

> and is there a way to ensure the transferConstraints doesn't change angle constraints unless perhaps there is a line segment parallel to the deleted one.

I don't know if this method should be changed or better delGeometry().

jnxd

2016-10-23 17:26

developer   ~0007419

> I don't know if this method should be changed or better delGeometry().

I would also suggest referring to the origin/root with GeoId 0, but it is also sensible to view it as a point on the x/horizontal axis. Also, I do not know if it is even possible since it could well be saved in the current format within FCStd files.

Perhaps it would also be a good idea to view each point as a separate entity every time, so transferConstraints() becomes unnecessary.

wmayer

2016-10-23 18:01

administrator   ~0007420

> I would also suggest referring to the origin/root with GeoId 0
That would be an evil thing because it will break all existing project files. I think I will add a new method that deletes at least angle and point-on-curve constraints if one of the associated geometries gets deleted.

> Perhaps it would also be a good idea to view each point as a separate entity every time, so transferConstraints() becomes unnecessary.
Sorry but can you explain your idea in more details, please?

jnxd

2016-10-23 18:57

developer   ~0007421

The first idea is purely cosmetic, but I guess it would be better to refer to the origin with a separate Id than the hor. axis (currently both are -1), and the most natural choice is 0. Any new element would then have to be referred to with and Id starting from 1.

For the second idea, every time an element A is created, create separate point elements for each new point created (currently there can be up to three new points, B, C, D, one for each pos of an arc or such elements). Let us call them nodes to differentiate them from the elements' pos points. Any point-based constraint, like distance or coincident, uses these nodes. There are perhaps additional constraints for coincidence between the pos points of A and B, C, D. Whenever a point is clicked on, the node is selected. When a coincident constraint is added between separate nodes, one of them is deleted and all constraints move to the surviving one. When a coincidence constraint between two elements is deleted, a new node is created, but we do not allow deletion of the constraint if it leaves the node without any geometry on it. If an element is deleted, any node on it not also coincident on a point of another element is also deleted.

Since transferConstraint() is used in delGeometry() to move the constraints from the point to be deleted (since the element it was going to be referred to is deleted), the new method renders it unnecessary since the constraints would already be on the nodes. The deleteConstraintsonPoint() function also reduces to removing precisely one coincidence constraint between the node and the point.

wmayer

2016-10-24 11:41

administrator   ~0007425

> The first idea is purely cosmetic ...
In FreeCAD the geo id of an external geometry is always negative. The axes and the root point are also considered as external geometry and thus I think their geo ids should also be negative to be consistent. However, as said before this will invalidate any existing projects where one of these geometries is used because the geo id is written into the project file.

Maybe it's best to discuss this topic further in the development forum.

Kunda1

2017-01-12 14:03

administrator   ~0007668

Last edited: 2017-02-15 14:36

View 2 revisions

> Maybe it's best to discuss this topic further in the development forum.
 
posted to http://forum.freecadweb.org/viewtopic.php?f=10&t=19814

Edit: previous thread I just found https://forum.freecadweb.org/viewtopic.php?f=10&t=18117

Kunda1

2017-01-30 15:57

administrator  

abdullah

2017-05-21 21:41

manager   ~0009075

https://github.com/FreeCAD/FreeCAD/pull/768

wmayer

2017-05-31 22:08

administrator   ~0009231

https://github.com/FreeCAD/FreeCAD/commit/1a33f3bf9913ca23effdb1d29bad900d638be0a5

Related Changesets

FreeCAD: master 1a33f3bf

2017-05-21 16:28:46

Abdullah Tahiri


Committer: wmayer Details Diff
Fixes 0002735 Affected Issues
0002735
mod - src/Mod/Sketcher/App/SketchObject.cpp Diff File

Issue History

Date Modified Username Field Change
2016-10-12 17:54 MajorTesla New Issue
2016-10-12 17:54 MajorTesla File Added: FreeCAD-incorrect-angle.jpg
2016-10-13 04:18 jnxd Note Added: 0007361
2016-10-15 18:33 normandc Project FreeCAD => Sketcher
2016-10-19 05:34 jnxd File Added: bug_2735_replicate.FCMacro
2016-10-19 05:43 jnxd Note Added: 0007394
2016-10-19 15:33 wmayer Status new => confirmed
2016-10-19 17:05 wmayer Note Added: 0007398
2016-10-20 04:27 jnxd Note Added: 0007406
2016-10-20 04:28 jnxd Note Edited: 0007406 View Revisions
2016-10-23 16:29 wmayer Note Added: 0007417
2016-10-23 16:46 wmayer Note Added: 0007418
2016-10-23 17:26 jnxd Note Added: 0007419
2016-10-23 18:01 wmayer Note Added: 0007420
2016-10-23 18:57 jnxd Note Added: 0007421
2016-10-24 11:41 wmayer Note Added: 0007425
2017-01-12 14:03 Kunda1 Note Added: 0007668
2017-01-30 15:57 Kunda1 File Added: FreeCAD-incorrect-angle-sm.jpg
2017-01-30 15:57 Kunda1 File Deleted: FreeCAD-incorrect-angle.jpg
2017-02-15 05:54 kkremitzki Target Version => 0.17
2017-02-15 14:36 Kunda1 Note Edited: 0007668 View Revisions
2017-05-21 21:38 abdullah Assigned To => abdullah
2017-05-21 21:38 abdullah Status confirmed => assigned
2017-05-21 21:41 abdullah Note Added: 0009075
2017-05-31 22:08 wmayer Status assigned => closed
2017-05-31 22:08 wmayer Resolution open => fixed
2017-05-31 22:08 wmayer Fixed in Version => 0.17
2017-05-31 22:08 wmayer Note Added: 0009231
2017-06-21 20:34 Kunda1 Changeset attached => FreeCAD master 1a33f3bf