Notes 

(0019882)

dbv

20120306 14:41




(0019883)

abv

20120306 15:02


I believe the current fix can cause infinite loop; just imagine that Eps = 0. In practice, it should never be 0., but can be DBL_MIN if Knots(i1) = 0.
I suggest to modify instead a way Eps is computed:
Standard_Real Eps = Max (Epsilon(Knots(i1)), Epsilon(Knots(i)));
Naturally, for optimization we can reuse value of Epsilon computed for previous knot on each cycle. Note that it is not necessary to take Abs() of the knot value, as function Epsilon() always returns positive number (see implementation in Standard_Real.hxx).
Besides, it is strange to see factor 1.1 multiplying Eps: by the very nature of Eps, all values below it should vanish when added to Knot(i). I suggest either removing that factor or (perhaps better) replacing it by 2. 



1. The warning of 0 was in the original bug report  the code of Epsilon() seems to ensure this does not ever happen. Otherwise, theoretically Max (Epsilon(Knots(i1)), Epsilon(Knots(i))) can also be zero.
2. 2 vs 1.1 is good.
3. Did not get a comment regarding Abs(). 


(0019943)

abv

20120313 10:29


I do not like while() here because it is not clear how many cycles it will take in worst case. If it is never infinite, from the same logic one cycle should be always sufficient.
I do not like 1.1 since I do not understand which effect additional 0.1 can have if we know that values less than Eps yield no change in the value of the Knot(i1) when added to it (from definition of Eps). Sorry, I am not an expert in discrete math and FPE...
So why we need to replace "if" by "while"?
How can we be sure that it is safe and works in all cases?
Should not we just replace 1.1 by 2?
Can you share the values of knots and epsilon at which the problem happens on your case?
I still believe the cycle is dangerous. Consider the possible case if Knot(i1)=0 (hence Epsilon = DBL_MIN) and Knot(i) = 1. Note that even this is invalid situation, nothing guarantees that Knot(i) > Knot(i1) in the input array. It is better to prevent this function from hanging up even on invalid data.
Perhaps the correct code would be:
if (Knots(i)  Knots(i1) <= Eps)
Knots(i) = Knots(i1) + Eps; 



The suggested code does not work as is  CheckCurveData() in Geom[2d]_BSplineCurve.cxx (or CheckSurfaceData() in Geom_BSplineSurface.cxx) fail as they use the same test and the difference Knots(i)Knots(i1) is exactly Eps.
However, the approach to add to Knots(i1) (not to Knots(i)) is reasonable so I reworked the code, using it. Note however that Knots(i)=Knots(i1)+1.1*Eps does not work (even if Eps is ~8e16). There must be some issues in mixing multiplication and addition, so single iteration is not enough. I reworked the code as follows:
> if (Knots(i)  Knots(i1) <= Eps) {
> //increment Knots(i) until the condition has been satisfied  see 0022989 for details
> Standard_Real anInc = Max (Eps, DBL_MIN);
> Knots(i) = Knots(i1) + anInc;
> while (Knots(i)  Knots(i1) <= Eps)
> Knots(i) += anInc;
> }
This avoids multiplication and ensures incrementation (Eps >= DBL_MIN), and thus no infinite loop.
Now to address your question:
the original 6.5.2 code fails here:
Knot(i1): 6.2831853071795853
Knot(i) : 6.2831853071795853
(not they are equal in VS IDE  likely difference is beyond the last digit).
Eps: 8.8817841970012523e016
After Knot += 1.1*Eps, Knot(i):6.2831853071795862
P.S. The new version will be attached. 


(0019978)

abv

20120315 10:07


Ok, now see the point: the distance should be not Eps, but greater. OCCT provides function NextAfter exactly for this, so my variant is:
if (Knots(i)  Knots(i1) <= Eps)
Knots(i) = NextAfter (Knots(i1) + Eps, RealLast());
This fix seems to work on your case (VS 2008) 


(0019979)

abv

20120315 10:08


The fix submitted to branch CR22989, please test 



NextAfter (Knots(i1) + Eps, RealLast()); is good for nonnegative Knots(i1). The original test case proceeds with it successfully.
However, for negative ones another code branch in NextAfter() is used which is not used in Epsilon(). So it must be investigated and better a test case created before committing the change. I can try to do this within a few days, unless you can do on your own.
In any case we better leave a bug id close to this modification, so that can consult these notes in the future if needed. 


(0020008)

mkv

20120315 20:03


Dear BugMaster,
Workbench KAS:dev:mkv22989occt was created from git branch CR22989
(and mkv22989products from svn trunk) and compiled on Linux platform.
Test case for this bug is chl/934/V8. It is OK.
There are not regressions in mkv22989products regarding to KAS:dev:products20120306opt
See results in /QADisk/occttests/results/KAS/dev/mkv22989products_15032012/lin
See reference results in /QADisk/occttests/results/KAS/dev/products20120306opt_07032012/lin
See test cases in /QADisk/occttests/tests/ED
N.B. In order to launch testing case you can make use the following instructions
http://doc/doku.php?id=occt:certification [^] 



To respond to my note above.
I confirm that the fix suggested by Andrey works for negative Knots as well. I attach a test case for that (edgenosameparameterneg.brep) 



