MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0030020Community[OCCT] OCCT:Modeling Datapublic2018-08-07 09:152018-10-16 16:02
Reporterdenix56 
Assigned Tomsv 
PrioritynormalSeverityminor 
StatusassignedResolutionopen 
PlatformWindowsOSVC++ 2015OS Version64 bit
Product Version[OCCT] 7.3.0 
Target VersionFixed in Version 
Summary0030020: Incorrect bspline visualization
DescriptionWhen I draw the spline as a line where the fit points (we`ll number it 1, 2, 3) are located in order 1, 3, 2 and vusialize it with VTK using IVtkTools_ShapeDataSource, the connection between points 2 and 3 is not drawn.

It is caused by the bug in GCPnts_TangentialDeflection::PerformCurve, where the check isLine returns true and only first and last points are connected.

The patch to fix this bug is provided.
Steps To Reproduce1. Define 3d fit points that lie on the line.
2. Interpolate them in order (1, 3, 2)
3. Create edge from spline
4. Visualize using IVtkTools_ShapeDataSource

# Draw script 1
set F [open pnts w]
puts $F "3 3d"
puts $F "0 0 0"
puts $F "2 0 0"
puts $F "1 0 0"
close $F
interpol c pnts
file delete pnts
crvtpoints r c 0.1 1

# Draw script 2
bsplinecurve c 2 2 0 3 146.299 3 17 -38 0 1 165.693 -38.4947 0 1 21.7888 -38 0 1
crvtpoints r c 0.4 12
TagsNo tags attached.
Test case number
Attached Filespatch file icon GCPnts_TangentialDeflection.patch (2,158 bytes) 2018-08-07 09:15
patch file icon GCPnts_TangentialDeflectionV2.patch (2,783 bytes) 2018-08-07 10:55
patch file icon GCPnts_TangentialDeflectionV3.patch (2,524 bytes) 2018-08-07 18:28
patch file icon GCPnts_TangentialDeflectionV4.patch (3,091 bytes) 2018-08-08 17:11
patch file icon GCPnts_TangentialDeflectionV5.patch (3,423 bytes) 2018-08-08 17:16
txt file icon bug_example.txt (76 bytes) 2018-08-08 18:00
patch file icon GCPnts_TangentialDeflectionV6.patch (5,678 bytes) 2018-08-09 08:57
? file icon incmesh_c7_21.brep (6,311 bytes) 2018-08-09 19:37
? file icon incmesh_c7_21_1.draw (1,855 bytes) 2018-08-09 20:08
png file icon crvtpoints_master.PNG (6,575 bytes) 2018-08-09 20:10
png file icon crvtpoints_patch.PNG (7,362 bytes) 2018-08-09 20:10
patch file icon FixSplineDiscret.patch (30,175 bytes) 2018-09-25 11:29
patch file icon FixSplineDiscretV2.patch (30,326 bytes) 2018-09-25 11:38
zip file icon patches.zip (8,961 bytes) 2018-09-28 18:01
patch file icon BRepMesh_DiscretFactory.cxx.patch (1,015 bytes) 2018-10-16 14:15

- Relationships

-  Notes
(0078450)
denix56 (reporter)
2018-08-07 11:00

Fixed incorrect delta in V2
(0078451)
msv (developer)
2018-08-07 11:19

The fix makes the algorithm insert 4 points in the result. Though it is enough to insert only 3 points.
(0078454)
denix56 (reporter)
2018-08-07 11:44
edited on: 2018-08-07 11:46

No, there will be only 3 points, the first, then 1 in loop [2, minNbPoints), where minNbPoints == 3 and the last one

Or maybe we talk about different lines

The old algorithm inserted more points then needed.

(0078456)
git (administrator)
2018-08-07 12:01

Branch CR30020 has been created by msv.

SHA-1: 47bfc98f1134be50f788210384e0c8e6cc9fbe06


Detailed log of new commits:

Author: msv
Date: Tue Aug 7 11:47:59 2018 +0300

    0030020: Incorrect bspline visualization
    
    Let's we have a spline having fit points located on a direct line (let's number them 1, 2, 3), but they are located in order 1, 3, 2. There was a bug in GCPnts_TangentialDeflection that only first and last points were put in the result. Now more point is put, so that the result polygon made back movement.
    
    The command crvtpoints has been made to accept angular value in degrees instead of radians.
(0078457)
msv (developer)
2018-08-07 12:07

I have prepared the branch for integration. It will be certified.
(0078466)
msv (developer)
2018-08-07 15:38

Jenkins job is CR30020-master-msv. It shows regressions that must be analyzed. The branch did not pass tests.
(0078472)
denix56 (reporter)
2018-08-07 17:40

Where can I see which tests failed?
(0078476)
denix56 (reporter)
2018-08-07 18:15
edited on: 2018-08-07 18:31

gp_XYZ V3 = MiddlePoint.XYZ() - prevMiddlePoint.XYZ();

            if(IsLine && IsSequential)
           {
                Standard_Real L3 = V3.Modulus();
                IsSequential = (L3 <= L1);
            }
           }
          prevMiddlePoint = MiddlePoint;

Why was it removed from branch?
It will not work correctly without it.

One thing that could be done is to move inside if(IsLine && IsSequential)

Could you please delete old files& I cannot do it (or don`t see the button)

Uploaded patch V3

(0078486)
msv (developer)
2018-08-08 12:41

The test results can be seen only in our intranet. However, if the used data model is not confidential I can provide the data and steps to reproduce the regression.
(0078487)
msv (developer)
2018-08-08 12:44

I removed L3 from the branch by logical reasons. I thought that we must measure the distance of the middle point always from the starting point. Otherwise, it is not logical to compare the distance of some insiding segment with the distance between start and end points.
(0078488)
msv (developer)
2018-08-08 12:45

Anyway, I will try your solution on the tests with regressions to see if it works.
(0078489)
msv (developer)
2018-08-08 12:48

The most markable regression can be reproduced using bug 0022304.
(0078490)
denix56 (reporter)
2018-08-08 14:40

We compare the distances between (2 middle points) and (middle poin and start point) to detect if the current middle point lies outside (first - last).
The comparison of L1 and L2 does not gie us such information, because l2 could be less than l1, but point lies outside, i.e. 2--1-------3
(0078494)
msv (developer)
2018-08-08 16:05

I see your point. But then the following case will not work: 1----2--4--3. To respect both cases it is needed to check both conditions L2<=L1 && L3<=L1.
(0078495)
msv (developer)
2018-08-08 16:10

BTW, I found out the cause of regressions. If the firstu and lastu delimit one interval, the conditions of rejecting neighboring intervals [...,firstu] and [lastu,...] does not work, and then both L2 and L3 can be much greater than L1. It leads to isSequential == false evel for quite sequential points. So, it is needed to elaborate more accurate interval rejecting in the beginning of the loop "for (i = 1; i <= NbInterv && IsLine; ++i)".
(0078496)
git (administrator)
2018-08-08 16:32

Branch CR30020 has been updated by msv.

SHA-1: 0ba065098f0056bedcd4e2d669b7dd7f023fca80


Detailed log of new commits:

Author: msv
Date: Wed Aug 8 16:30:25 2018 +0300

    # fighting against regressions

Author: msv
Date: Wed Aug 8 12:15:08 2018 +0300

    #return back to original patch by denix56

(0078497)
msv (developer)
2018-08-08 16:35

Dear denix56, I think that prevMiddlePoint must be initialized before entering the loop on intervals, so that it did not reset to the start point with each interval.

I have updated the branch and launched tests. Could you review it meanwhile?
(0078498)
git (administrator)
2018-08-08 16:37

Branch CR30020 has been updated forcibly by msv.

SHA-1: 1d29c498ca9317d99cc3d7510b512f60208f46da
(0078499)
denix56 (reporter)
2018-08-08 17:18
edited on: 2018-08-08 17:21

I added V5, where fixed incorrect visualization when spline is nearly but not linear and point 2 is outside and located far from 1 and 3.

Fixed by replacement sun check with the cos one (problem happens when angle is > 90)

Reviewed current branch, everything`s look ok.

(0078501)
msv (developer)
2018-08-08 17:49

Could you attach the shape to reproduce the problem in your last message? You can use the method GeomTools::Write to obtain a file.
If so, then patch V5 could be also taken.
(0078503)
denix56 (reporter)
2018-08-08 18:00

If I`ve done it right, you will see that connection to 2nd point disappears.
(0078505)
msv (developer)
2018-08-08 18:24
edited on: 2018-08-08 18:27

I have tried this curve, it is always tessallated well, even without patch V5.

(0078506)
msv (developer)
2018-08-08 18:26

Which parameters do you pass to the algorithm?
(0078508)
git (administrator)
2018-08-08 18:43

Branch CR30020 has been updated by msv.

SHA-1: d9a0f9af17f0f4ab63934f0ef2d80253e947c80b


Detailed log of new commits:

Author: msv
Date: Wed Aug 8 17:54:58 2018 +0300

    # accomodate patch V5

(0078509)
denix56 (reporter)
2018-08-08 18:43

I use bigger dev_coeff for IVtkOCC_ShapeMesher 0.001
(0078510)
msv (developer)
2018-08-08 18:58

I do not use VTK, and it is hard for me derive GCPnts_TangentialDeflection parameters from IVtkOCC_ShapeMesher parameters. Could you break in the algorithm to see the parameters?
(0078511)
denix56 (reporter)
2018-08-08 19:19

curvatureDeflection = 0.41189937809767357
angularDeflection = 0.20943951023931953
MinimumOfPoints = 2
UTol = 9.9999999999999986e-10
theMinLen = 9.9999999999999995e-08
(0078512)
msv (developer)
2018-08-08 21:32

OK, thanks, now I could reproduce the bug.
(0078513)
msv (developer)
2018-08-08 21:44

Tests have not been passed with patch v5. A lot of errors and differences in output meshes.
(0078514)
denix56 (reporter)
2018-08-08 23:49
edited on: 2018-08-09 08:12

Only one thing that could be changed is to compare the dot product with greater or equal sign (if points are same dot product return 0)

Do you any ideas what causes these problems?)

If it`s possible, could you provide the small example (as I did) of what error or difference produced?

(0078515)
denix56 (reporter)
2018-08-09 08:58

Uploaded V6 patch, some improvements and maybe it will fix errors (don`t know).
(0078542)
git (administrator)
2018-08-09 19:13

Branch CR30020 has been updated by msv.

SHA-1: 9926564df2deedee6b2c0f07f0d3393545e31861


Detailed log of new commits:

Author: msv
Date: Thu Aug 9 19:11:24 2018 +0300

    # patch V6

(0078543)
msv (developer)
2018-08-09 19:41

With the patch V6 many regressions were gone. However, some remain. I have attached a face from one test data model. It is meshed and visualized on master version, but fails with the patch.

Commands to reproduce in Draw:
restore incmesh_c7_21.brep f
incmesh f 0.1
trinfo f
(0078544)
msv (developer)
2018-08-09 20:13

From the face above, the problem edge is the first one. I have attached its curve in the last file incmesh_c7_21_1.draw.
The master version tesselates its end in another way than in the patch. See attached snapshots crvtpoints_master.PNG and crvtpoints_patch.PNG.
This causes the error "self intersecting wire" in the meshing algorithm.
(0078547)
denix56 (reporter)
2018-08-09 22:06

Unfortunately, I will away from my laptop till the end of the next week.
Could you please attach file so that I could read it with geom_tools read (if it is possible)?
(0078550)
msv (developer)
2018-08-10 09:54

File is attached. To read by GeomTools just remove the first line from .draw file. The .brep file is to be read by BRepTools.
I will be away next 3 weeks.
(0078710)
denix56 (reporter)
2018-08-21 19:58
edited on: 2018-08-22 10:19

First look showed that on this curve some points are not sequential (or at least D0 returns such points). It happens int the end of the algorithm when we insert additional points. On some of them the middlePoint is located after P1 and P2, causing self-intersections. It might be problem with either the data or the D0 function, not algorithm.

I have plotted the points from draw file using some web tools - it shows that points form self-intersecting curve.
The tests passed before due to the incorrect work of the algorithm (sin was used instead of cos)

(0079349)
denix56 (reporter)
2018-09-24 11:41

Any updates on this issue?
(0079366)
msv (developer)
2018-09-24 14:48

Sorry for delay, I have many work in other projects.

Indeed, this curve is self-intersceting and has many forward-back movements along it.
You are right, in the master version the algorithm behaves in different way and occasionally this face is triangulated without error.

Anyway, this fix introduces formal regression. It would be great if we could make workaround for this case. E.g., we can make two modes of work of the patched algorithm, and call it from BRepMesh using the old behavior.
(0079367)
denix56 (reporter)
2018-09-24 16:05
edited on: 2018-09-24 16:27

Calling from BRepMesh using the old behavior will produce incorrect results on good meshes while we create workaround for incorrect mesh.

I think that fixing that test file will be much better solution.

(0079372)
msv (developer)
2018-09-24 17:48

Please provide a new test case that proves the necessity of this fix.
In the steps to reproduce only the case of direct calling the patched algorithm is provided. And now I mean that we need a test case that involves incmesh.
(0079373)
msv (developer)
2018-09-24 17:50

Usually we avoid fixing input test data, because they provide true test case from a customer. Altering test file and patching the algorithm we can introduce a regression in some user application.
(0079383)
denix56 (reporter)
2018-09-25 11:31
edited on: 2018-09-25 11:32

I`ve added new patch that contains this workaround - by default, ocad will use old behavior in brep_mesh, and if user want to use new beahavior there is an argument to change it.
Also there is note about the fact that old behavior might cause incorrect results and provided only for backward compatibility.

(0079469)
msv (developer)
2018-09-28 12:11

How did you get the patch file? I cannot apply your patch:
D:\git\occt1>git apply FixSplineDiscretV2.patch
error: corrupt patch at line 17

I use git version 2.16.1.windows.4.
Please provide a valid patch file.
(0079493)
denix56 (reporter)
2018-09-28 18:02

It seems i have some problems with creation of the single patch, so I provided archive with separate patches for each file.
(0079738)
msv (developer)
2018-10-05 19:48

These patches also have errors to apply. Eventually, I have managed to apply them. For that I had to manually change the lines like:
--- "a/D:\\dev-tools\\opencascade-7.3.0\\tmp\\BRepMesh_FastDiscret.hxx"
+++ "b/D:\\dev-tools\\opencascade-7.3.0\\src\\BRepMesh\\BRepMesh_FastDiscret.hxx"
to
--- "a/src\\BRepMesh\\BRepMesh_FastDiscret.hxx"
+++ "b/src\\BRepMesh\\BRepMesh_FastDiscret.hxx"

BTW, your patch misses the changes in the file BRepMesh_DiscretFactory.cxx.

In substance, the new parameter TDOldBehaviour looks ugly. Please give me a while for thinking about a way to avoid such impacting the algorithms.

Again, I repeat my request for test file to reproduce the initial problem using mesh algorithm. I highly need it to create a non-regression test.
(0079739)
git (administrator)
2018-10-05 20:03

Branch CR30020 has been updated by msv.

SHA-1: 897bd7f6efd4ad2bed98b5741b1dd78ca22ccb17


Detailed log of new commits:

Author: msv
Date: Fri Oct 5 20:03:04 2018 +0300

    # patch with leaving old behavior, and adding new behavior by option

(0079740)
git (administrator)
2018-10-05 20:05

Branch CR30020 has been updated forcibly by msv.

SHA-1: 83bc18b4f0769ddc2e0a177a3a31adddaf4d15bd
(0079769)
git (administrator)
2018-10-08 10:14

Branch CR30020 has been updated by msv.

SHA-1: f9f5db1264e7efa8ff8e82ebb64cdeeb85478d76


Detailed log of new commits:

Author: msv
Date: Mon Oct 8 10:14:00 2018 +0300

    # correct test cases

(0080011)
denix56 (reporter)
2018-10-16 14:15
edited on: 2018-10-16 16:02

Sorry for delay with answer, has lots of other work to do.
Added patch for discretfactory.cxx


- Issue History
Date Modified Username Field Change
2018-08-07 09:15 denix56 New Issue
2018-08-07 09:15 denix56 Assigned To => msv
2018-08-07 09:15 denix56 File Added: GCPnts_TangentialDeflection.patch
2018-08-07 10:55 denix56 File Added: GCPnts_TangentialDeflectionV2.patch
2018-08-07 11:00 denix56 Note Added: 0078450
2018-08-07 11:19 msv Note Added: 0078451
2018-08-07 11:20 msv Steps to Reproduce Updated View Revisions
2018-08-07 11:44 denix56 Note Added: 0078454
2018-08-07 11:45 denix56 Note Edited: 0078454 View Revisions
2018-08-07 11:45 denix56 Note Edited: 0078454 View Revisions
2018-08-07 11:46 denix56 Note Edited: 0078454 View Revisions
2018-08-07 11:46 denix56 Note Edited: 0078454 View Revisions
2018-08-07 12:01 git Note Added: 0078456
2018-08-07 12:07 msv Note Added: 0078457
2018-08-07 12:07 msv Status new => assigned
2018-08-07 15:38 msv Note Added: 0078466
2018-08-07 17:40 denix56 Note Added: 0078472
2018-08-07 18:15 denix56 Note Added: 0078476
2018-08-07 18:16 denix56 Note Edited: 0078476 View Revisions
2018-08-07 18:28 denix56 File Added: GCPnts_TangentialDeflectionV3.patch
2018-08-07 18:28 denix56 Note Edited: 0078476 View Revisions
2018-08-07 18:31 denix56 Note Edited: 0078476 View Revisions
2018-08-08 12:41 msv Note Added: 0078486
2018-08-08 12:44 msv Note Added: 0078487
2018-08-08 12:45 msv Note Added: 0078488
2018-08-08 12:48 msv Note Added: 0078489
2018-08-08 14:40 denix56 Note Added: 0078490
2018-08-08 16:05 msv Note Added: 0078494
2018-08-08 16:10 msv Note Added: 0078495
2018-08-08 16:32 git Note Added: 0078496
2018-08-08 16:35 msv Note Added: 0078497
2018-08-08 16:37 git Note Added: 0078498
2018-08-08 17:11 denix56 File Added: GCPnts_TangentialDeflectionV4.patch
2018-08-08 17:16 denix56 File Added: GCPnts_TangentialDeflectionV5.patch
2018-08-08 17:18 denix56 Note Added: 0078499
2018-08-08 17:19 denix56 Note Edited: 0078499 View Revisions
2018-08-08 17:21 denix56 Note Edited: 0078499 View Revisions
2018-08-08 17:49 msv Note Added: 0078501
2018-08-08 18:00 denix56 File Added: bug_example.txt
2018-08-08 18:00 denix56 Note Added: 0078503
2018-08-08 18:24 msv Note Added: 0078505
2018-08-08 18:26 msv Note Added: 0078506
2018-08-08 18:27 msv Note Edited: 0078505 View Revisions
2018-08-08 18:43 git Note Added: 0078508
2018-08-08 18:43 denix56 Note Added: 0078509
2018-08-08 18:58 msv Note Added: 0078510
2018-08-08 19:19 denix56 Note Added: 0078511
2018-08-08 21:32 msv Note Added: 0078512
2018-08-08 21:34 msv Steps to Reproduce Updated View Revisions
2018-08-08 21:44 msv Note Added: 0078513
2018-08-08 23:49 denix56 Note Added: 0078514
2018-08-09 08:12 denix56 Note Edited: 0078514 View Revisions
2018-08-09 08:57 denix56 File Added: GCPnts_TangentialDeflectionV6.patch
2018-08-09 08:58 denix56 Note Added: 0078515
2018-08-09 19:13 git Note Added: 0078542
2018-08-09 19:37 msv File Added: incmesh_c7_21.brep
2018-08-09 19:41 msv Note Added: 0078543
2018-08-09 20:08 msv File Added: incmesh_c7_21_1.draw
2018-08-09 20:10 msv File Added: crvtpoints_master.PNG
2018-08-09 20:10 msv File Added: crvtpoints_patch.PNG
2018-08-09 20:13 msv Note Added: 0078544
2018-08-09 22:06 denix56 Note Added: 0078547
2018-08-10 09:54 msv Note Added: 0078550
2018-08-21 19:58 denix56 Note Added: 0078710
2018-08-22 10:19 denix56 Note Edited: 0078710 View Revisions
2018-09-24 11:41 denix56 Note Added: 0079349
2018-09-24 14:48 msv Note Added: 0079366
2018-09-24 16:05 denix56 Note Added: 0079367
2018-09-24 16:26 denix56 Note Edited: 0079367 View Revisions
2018-09-24 16:27 denix56 Note Edited: 0079367 View Revisions
2018-09-24 17:48 msv Note Added: 0079372
2018-09-24 17:50 msv Note Added: 0079373
2018-09-25 11:29 denix56 File Added: FixSplineDiscret.patch
2018-09-25 11:31 denix56 Note Added: 0079383
2018-09-25 11:32 denix56 Note Edited: 0079383 View Revisions
2018-09-25 11:38 denix56 File Added: FixSplineDiscretV2.patch
2018-09-28 12:11 msv Note Added: 0079469
2018-09-28 18:01 denix56 File Added: patches.zip
2018-09-28 18:02 denix56 Note Added: 0079493
2018-10-05 19:48 msv Note Added: 0079738
2018-10-05 20:03 git Note Added: 0079739
2018-10-05 20:05 git Note Added: 0079740
2018-10-08 10:14 git Note Added: 0079769
2018-10-16 14:15 denix56 Note Added: 0080011
2018-10-16 14:15 denix56 File Added: BRepMesh_DiscretFactory.cxx.patch
2018-10-16 16:02 denix56 Note Edited: 0080011 View Revisions


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker