MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0029115Community[OCCT] OCCT:Modeling Datapublic2017-09-15 12:072018-04-16 15:59
ReporterRoman Lygin 
Assigned Tomsv 
PriorityhighSeverityminor 
StatusresolvedResolutionno change required 
PlatformOSOS Version
Product Version[OCCT] 7.2.0 
Target Version[OCCT] 7.4.0*Fixed in Version 
Summary0029115: [Regression] GeomAdaptor mistakenly reports non-periodic curves to be periodic
Description7.2.0 introduced the following regression - a synthetic reproducer is below:

gp_Circ aCirc (gp_Ax2(), 1);
Handle (Geom_Circle) aCircle = new Geom_Circle (aCirc);
Handle (Geom_Curve) aCurve = new Geom_TrimmedCurve (aCircle, 0 , M_PI);

GeomAdaptor_Curve anAdaptor (aCurve);

assert (!anAdaptor.IsPeriodic());
Steps To ReproduceSee description
TagsNo tags attached.
Test case number
Attached Files

- Relationships

-  Notes
(0070584)
msv (developer)
2017-09-15 14:43

We cannot consider this issue as a bug. Moreover it cannot be considered as a regression.
The changes made in the scope of the bug #28724 are conscious. And these changes are described in the Upgrade guide.

So, I propose to close this issue.
(0070588)
Roman Lygin (developer)
2017-09-15 15:03

It is up to you, so please decide as you deem appropriate but I find the logic broken.

Geometrically-wise, periodicity is a more strict property than closedness, so closedness is a mandatory condition of periodicity. The curve can be closed but not periodic. But if the curve is periodic then it is closed.

The fix breaks this geometrical principle and results in periodic but open curve, which is weird.

#28724 is not publicly accessible so no clues what's in there. Release Notes just barely mentions the fix, no background/justification either.
(0070595)
msv (developer)
2017-09-15 16:04

You can read commit message corresponding to the fix of 28724.
Also, you can read upgrade guide, as I told.
Periodicity has no common with closeness.
Geometry is considered periodic if by adding the period to the parameter we obtain the same 3D point. And it is the case for a part of circle.
(0070654)
Roman Lygin (developer)
2017-09-18 17:43

I find the statement on periodicity having nothing in common with closedness to be just plain wrong.
In CAD world it is a de-facto convention that periodicity is a particular case of closedness, i.e. is a more strict
requirement than closedness. That is a curve/surface cannot be periodic until it is closed.
Here are a few resources:
- http://docs.mcneel.com/rhino/5/help/en-us/popup_moreinformation/periodic.htm [^]
- https://knowledge.autodesk.com/support/maya-lt/learn-explore/caas/CloudHelp/cloudhelp/2015/ENU/MayaLT/files/NURBS-overview-Periodic-closed-and-open-geometry-htm.html [^]
- http://support.ptc.com/help/creo/creo_pma/usascii/index.html#page/surfacing/style/About_Closed_Curves.html [^]

Generic resources:
- https://math.stackexchange.com/questions/1477058/closed-periodic-curve [^]

Similar concepts are reflected in ACIS and Parasolid which are kernels underlying the greatest number of CAD systems.

Challenging that assumption can be way too confusing for the users and leading to a lot of misunderstanding.

The Geom[2d]Adaptor_Curve (as the name suggests) is aimed to implement an adaptor design
pattern (https://en.wikipedia.org/wiki/Adapter_pattern [^]) adapting the Geom[2d]_Curve to a generic curve-like class which
many surface modeling algorithm would accept. This adaptor represents a blackbox curve that has a range [a,b], can be evaluated
at parameter t in that range

 Here is the pseudo-code that did work until 7.1.0 (including):

function Handle(Geom_Surface) MakePipe (const Adaptor3d_Curve& thePath, <parameters>)
{
    //use GeomFill_Pipe which returns non-periodic surfaces even for periodic paths
    Handle(Geom_Surface) aRes = aGeomFillAlgo.Result();
    if (thePath.IsPeriodic() && !aResult.IsVPeriodic()) {
        //convert to periodic
        ShapeCustom_Surface aCS (aRes);
        aRes = aCS.ConvertToPeriodic (Standard_False, aTol);
        assert (!aRes.IsNull());
    }
    return aRes;
}

Up to 7.2.0 thePath.IsPeriodic() returned true for really periodic curves, i.e. a periodic basis curve with range = period (e.g. [0,2*PI], [PI/2, 2.5*PI]).

Now with 7.2.0 thePath.IsPeriodic() returns true even for semi-circle ([0,PI]) leading to false positives.

A user's code which was quite legitimate in previous version of OCC stops working due to introducing a side-effect logic in Geom[2d]_Adaptor.
Now you basically force to revisit entire code base where GeomAdaptor could be used and revisit if periodicity assumptions are not longer valid.
This is just an example of why migration to each new OCC version could be pain in the neck and frightening.

I am offering you a real case feedback with hope to convince that design decisions in modeling algorithms have to be taken with a lot of care
and analysis of possible consequences. Established assumptions and conventions should not be neglected and overcome just to address a particular
(even if important) use case. You might prefer to introduce a special processing without affecting mainstream scenarios already incorporated
in your users codes.
Thanks,
(0070666)
nbv (developer)
2017-09-19 10:41
edited on: 2017-09-19 10:42

Dear Roman,
Thank you for your links to the User guides of other CAD-systems. However, I would like to draw your attention to the following:

1. As it has already been said by Mikhail (MSV) “Geometry is considered periodic if by adding the period to the parameter we obtain the same 3D point.” (please see the message 0029115:0070595).
2. All questions in the domain. I.e. if any circle is defined in the interval [PI/2, 3*PI/2] then we have the following situation. Let us consider some parameter (e.g. PI) associated with some 3D-point. If we add or subtract the period (2*PI) we will obtain the point which is out of the domain [PI/2, 3*PI/2]. By the same reason, this arc of circle cannot be considered as periodic, in fact, in spite of new 3D-point being the same as for previous parameter (PI).
3. Remark in item 2 is excepted by the fact that the current version of OCCT allows extending surfaces/curves out of their true domain. Yes, it leads to some problems. Therefore, the issue #23675 (P-curves of a face are out of the domain of the face) has been created in order to forbid to work out of domain (this bug is private and has not been integrated yet). However, the current version of OCCT allows it. Therefore, the point with parameter (-PI) on the circle (see item 2) can exist. Therefore, even above described circle can be considered as periodic.
So, despite this question being contentious, new behavior (entered in frame of the issue #28724) DOES NOT contradict the main OCCT-conception.
------
Now, let me give a few words about the fix #28724. All reasons are given in the commit message: “The main reason of the regression is that the Extrema algorithm finds the truth extrema point but cannot adjust it to the range of given circle. It is connected with the fact that Geom(2d)Adaptor_Curve::IsPeriodic() method returns false for given circle because adaptor contains a piece of the circle which is not closed.” I.e. the extrema point was found but cannot be adjusted to the true circle domain. After the fix, it is adjusted correctly.
-----
We agree that the fix leads to global changes in customer code. Therefore, we have written corresponding notes in Upgrade guide. And I regret but corresponding codes in customer applications shall be rewritten.
New example of the code can be taken from changes in “GeomFill_NSections.cxx” made in frame of the issue #28724. I.e. Use

const Standard_Real aParF = AC1.FirstParameter();
const Standard_Real aParL = AC1.LastParameter();
const Standard_Real aPeriod = AC1.IsPeriodic() ? AC1.Period() : 0.0;
if ((aPeriod != 0.0) && (Abs(aParL - aParF - aPeriod) < Precision::PConfusion()))


Instead of

if(AC1.IsPeriodic())


(0070711)
nbv (developer)
2017-09-20 13:42
edited on: 2017-09-20 13:46

After discussions, we decide

1. To return previous behavior of adaptors.
2. To add in Adaptor*-classes new methods (e.g. IsBasisPeriodic() or AdjustPeriodic()).
3. To use new methods in algorithms where analogical problem exists.

Additionally, new synthetic cases should be created to cover base low-level algorithms (such as, all extremas, intersector, classifier).

(0070716)
Roman Lygin (developer)
2017-09-20 18:18

That is about what I had in mind. Thank you guys.
(0070722)
msv (developer)
2017-09-21 02:08

I agree, please proceed.
However, if you add IsBasisPeriodic method you will be forced to add also the method BasisPeriod(), and may be others. I would prefer adding AdjustPeriodic method, which will do nothing in general case.
(0070724)
azv (developer)
2017-09-21 08:03

Dear Mikhail, Nikolay,

I'm very confused by the proposed solution. What is a purpose to implement such strange behavior?

In my opinion, methods like AdjustPeriodic() are applicable only if the adaptor returns true by IsPeriodic() methods. Unfortunately, in case of half circle (for example, in the range [2*pi, 3*pi]) IsPeriodic() should return false, thus there is no case to adjust.

Method IsBasisPeriodic() is more natural, but it brokes a role of adaptor to hide original curve from the user. Moreover, two methods IsPeriodic() and IsBasisPeriodic() will disorient a user which one should be used.

I also think that the algorithms (Extrema, for example) should take care of the parametric range of the curve. I mean, that the Extrema algorithms for the case above (half circle in the range [2*pi, 3*pi]) have to return parametric value in the given range, but not in a real domain of underlying circle [0, 2*pi].
(0070727)
msv (developer)
2017-09-21 09:40

Dear Artem,
I understand your confusion. However, Extrema has only instance of adapter to make decision about actions needed to complete its job, and we should provide it with convenient interface to do this job, and at the same time to minimize code duplication. For that, it is natural to insert the method of adjusting periodicity in the adapter. To avoid confusion with IsPeriodic returning false, we can name the new method something like TryPutPointInside. If you have another idea, please share it.
(0070728)
nbv (developer)
2017-09-21 09:46

Dear Artem,

Discussion about "IsPeriodic" is impasse (it can be continued infinitely). I think, all questions will be solved by the detailed documentation about new methods. So, these new methods will be able to be added and used in necessary algorithms.

> I also think that the algorithms (Extrema, for example) should take care of the parametric range of the curve. I mean, that the Extrema algorithms for the case above (half circle in the range [2*pi, 3*pi]) have to return parametric value in the given range, but not in a real domain of underlying circle [0, 2*pi].

Unfortunately, low-level extrema (which finds all solutions itself) works neither curve nor adaptor. It works with gp_circle. So, it cannot return the point in range different from [0,2*pi]. All adjustments are made in higher level.

Moreover, what about other algorithms? If we try to intersect periodic B-spline defined in "shifted" range with another B-spline. In the current version of OCCT, all points of found Walking-line will be adjusted. However, will it be done if both arguments are considered as not periodic? So, we must insert correspond piece of code in all algorithms and support it by the synthetic cases.
(0070729)
azv (developer)
2017-09-21 10:14

nbv >> Unfortunately, low-level extrema (which finds all solutions itself) works neither curve nor adaptor. It works with gp_circle. So, it cannot return the point in range different from [0,2*pi]. All adjustments are made in higher level.

That's what I tried to say above. Adaptor should not take care about the parameter of the point, the Extrema algorithm should do it.

nbv >> Moreover, what about other algorithms? If we try to intersect periodic B-spline defined in "shifted" range with another B-spline. In the current version of OCCT, all points of found Walking-line will be adjusted. However, will it be done if both arguments are considered as not periodic? So, we must insert correspond piece of code in all algorithms and support it by the synthetic cases.

Sorry, but your proposal leads to changing this code too. What are the benefits of your approach?
(0070730)
nbv (developer)
2017-09-21 10:24

> That's what I tried to say above. Adaptor should not take care about the parameter of the point, the Extrema algorithm should do it.

This solution leads to copy-paste the same code fragment in different places of OCCT. So, it is better to create some global function (e.g. as method of Adaptor* class) and to call it from these places. The main idea is that the new function must be global and available for main OCCT-algorithms. Therefore, Adaptor is a good place for it.


> Sorry, but your proposal leads to changing this code too. What are the benefits of your approach?

New solution has not any benefits. However, in the current MASTER, all points will be adjusted correctly.
(0071309)
abv (manager)
2017-10-09 14:22

Note discussion of apparently the same subject here: https://dev.opencascade.org/index.php?q=node/1155 [^]
(0074668)
git (administrator)
2018-03-19 18:37

Branch CR29115 has been created by nbv.

SHA-1: 023408bad3fef54bff393bea083214544bac987d


Detailed log of new commits:

Author: nbv
Date: Wed Feb 21 17:31:37 2018 +0300

    0029115: [Regression] GeomAdaptor mistakenly reports non-periodic curves to be periodic
    
    1. Section "Concept of periodicity applied in OCCT-algorithms" has been added in Doxigen-documentation
    
    2. Method IsPeriodic() of classes Geom(2d)_TrimmedCurve has changed its behavior. Please see the documentation about correspond methods.
    
    3. Methods IsUPeriodic() and IsVPeriodic() has changed their behavior.
    
    4. Methods Geom_SurfaceOfLinearExtrusion::UPeriod() and Geom_SurfaceOfRevolution::VPeriod() has changed their behavior.
    
    5. Method GeomLib::IsClosed(...) has been added in order to check closure of 3D/2D curve with some tolerance.
    
    6. Methods ShapeAnalysis_Curve::IsPeriodic(...), BRepBlend_HCurve2dTool::IsPeriodic(...), Contap_HCurve2dTool::IsPeriodic(...), IntPatch_HCurve2dTool::IsPeriodic(...), BRepBlend_HCurveTool::IsPeriodic(...) have been removed.
    
    7. Method
    
       void BOPTools_AlgoTools2D::AdjustPCurveOnFace (const TopoDS_Face& theF,
                                                      const Handle(Geom_Curve)& theC3D,
                                                      const Handle(Geom2d_Curve)& theC2D,
                                                      Handle(Geom2d_Curve)& theC2DA,
                                                      const Handle(IntTools_Context)& theContext = Handle(IntTools_Context)());
    
    has been removed.
    
    8. The following classes have become abstract (some methods have become pure virtual): Adaptor2d_Curve2d, Adaptor3d_Curve, Adaptor3d_Surface.
    
    9. Methods ChFiDS_ElSpine::IsClosed() GeomFill_SnglrFunc::IsClosed(), ProjLib_CompProjectedCurve::IsClosed(), ProjLib_CompProjectedCurve::IsPeriodic(), ProjLib_CompProjectedCurve::Period() have been created.
    
    10. Methods GeomLib::AllowExtend(...), GeomLib::AllowExtendUParameter(...) and GeomLib::AllowExtendVParameter(...) have been created.
    
    11. Interface of Geom_ConicalSurface::Apex(...) method has been corrected insignificantly.
    
    12. Method IntTools_Tools::IsClosed(...) has been removed.
(0074690)
git (administrator)
2018-03-20 10:58

Branch CR29115 has been updated forcibly by nbv.

SHA-1: 6d183b787422b4f46d221d933f7e638c5c348e87
(0074778)
git (administrator)
2018-03-22 19:34

Branch CR29115 has been updated forcibly by nbv.

SHA-1: f7d79f5f580ec4c4c6835473390a0ce7ec6f7fc0
(0074796)
git (administrator)
2018-03-23 10:14

Branch CR29115 has been updated forcibly by nbv.

SHA-1: 61a11f04a19fe1b891b550aaec86aebeba6cef77
(0074909)
git (administrator)
2018-03-26 10:53

Branch CR29115 has been updated forcibly by nbv.

SHA-1: 8f3a6b85efa1584dc5134d02228b314caac75777
(0074974)
git (administrator)
2018-03-27 17:26

Branch CR29115_1 has been created by nbv.

SHA-1: 0cf4e3d35f348b6aad6a08252a62459ebc91eb7b


Detailed log of new commits:

Author: nbv
Date: Wed Feb 21 17:31:37 2018 +0300

    0029115: [Regression] GeomAdaptor mistakenly reports non-periodic curves to be periodic
    
    1. Section "Concept of periodicity applied in OCCT-algorithms" has been added in Doxigen-documentation
    
    2. Method IsPeriodic() of classes Geom(2d)_TrimmedCurve has changed its behavior. Please see the documentation about correspond methods.
    
    3. Methods IsUPeriodic() and IsVPeriodic() has changed their behavior.
    
    4. Methods Geom_SurfaceOfLinearExtrusion::UPeriod() and Geom_SurfaceOfRevolution::VPeriod() has changed their behavior.
    
    5. Method GeomLib::IsClosed(...) has been added in order to check closure of 3D/2D curve with some tolerance.
    
    6. Methods ShapeAnalysis_Curve::IsPeriodic(...), BRepBlend_HCurve2dTool::IsPeriodic(...), Contap_HCurve2dTool::IsPeriodic(...), IntPatch_HCurve2dTool::IsPeriodic(...), BRepBlend_HCurveTool::IsPeriodic(...) have been removed.
    
    7. Method
    
       void BOPTools_AlgoTools2D::AdjustPCurveOnFace (const TopoDS_Face& theF,
                                                      const Handle(Geom_Curve)& theC3D,
                                                      const Handle(Geom2d_Curve)& theC2D,
                                                      Handle(Geom2d_Curve)& theC2DA,
                                                      const Handle(IntTools_Context)& theContext = Handle(IntTools_Context)());
    
    has been removed.
    
    8. The following classes have become abstract (some methods have become pure virtual): Adaptor2d_Curve2d, Adaptor3d_Curve, Adaptor3d_Surface.
    
    9. Methods ChFiDS_ElSpine::IsClosed() GeomFill_SnglrFunc::IsClosed(), ProjLib_CompProjectedCurve::IsClosed(), ProjLib_CompProjectedCurve::IsPeriodic(), ProjLib_CompProjectedCurve::Period() have been created.
    
    10. Methods GeomLib::AllowExtend(...), GeomLib::AllowExtendUParameter(...) and GeomLib::AllowExtendVParameter(...) have been created.
    
    11. Interface of Geom_ConicalSurface::Apex(...) method has been corrected insignificantly.
    
    12. Method IntTools_Tools::IsClosed(...) has been removed.
    
    13. The DRAW-commands "curveperiod" and "surfaceperiod" have been created. Please see help-system to obtain the information these commands.
    
    14. Several DRAW-commands have been created to provide work with adapters. They are defined in the file GeomliteTest_AdaptorCommands.cxx. Currently they allow reading the following adapter's properties: periodicity, closure and resolution.
    
    15. New test cases have been created.
(0074984)
nbv (developer)
2018-03-28 10:04

Dear Mikhail,

Please review CR29115_1 branch for OCCT and CR29115prod branch for OCCT-products.

Test results are here: http://jenkins-test-11.nnov.opencascade.com:8080/view/CR29115-CR29115prod_NBV/. [^]

There are some remarks:

1. About the messages 0029115:0071309 and 0029115:0070711. Currently, we allow trimmed curve/surface and adaptors being periodic if they are based on periodic curves. Any trim boundaries are not taken into account. This behavior has been documented.

2. Differences in images are found:

a. IMAGE sat doc_1 B8: B8.png differs
   IMAGE sat doc_1 F4: F4.png differs
   IMAGE sat load B3: B3.png differs
   IMAGE sat doc_2 A3: A3.png differs
   IMAGE sat doc_2 A1: A1.png differs
   IMAGE sat doc_2 A4: A4.png differs
   IMAGE sat doc_2 A2: A2.png differs
   ...

Seam-edge has been rotated. It is not regressions/improvements because seam-edges can take any locations.

b. IMAGE boolean bopfuse_complex J6: J6.png differs
   IMAGE boolean bfuse_complex M1: M1.png differs
   IMAGE boolean bfuse_complex K6: K6.png differs
   IMAGE boolean bcut_complex H8: H8.png differs
   IMAGE heal split_closed_faces F4: F4.png differs
   IMAGE heal split_continuity_advanced ZF1: ZF1.png differs
   IMAGE heal surface_to_bspline F6: F6.png differs
   ...

The reason of this behavior is explained in the issue #23675: P-curves of a face are out of the domain of the face. DBRep_IsoBuilder processes periodic and not periodic faces (which are out of the surface's domain) differently. So, the structure of the shape has not been changed. We just have another set of iso-lines. In my opinion, it is not a regression. Moreover, the new result is looked better.

c. IMAGE omf advanced_meshcut Z6: Z6.png differs
   IMAGE omf advanced_meshfuse ZB6: ZB6.png differs
   IMAGE omf advanced_meshfuse V5: V5.png differs
   
Meshing algorithm works differently for periodic and not-periodic faces. However, it is not a regression/improvement.

Other differences are explained in the commit message.

3. I kindly recommend you to start reviewing with documentation. It is really describes and explains all made changes.
(0075097)
msv (developer)
2018-04-02 11:28

Remarks to documentation:

dox/user_guides/modeling_data/modeling_data.md
- 790: must be "of their curves"
- 798: "In OCCT, a surface is set in parametric form"
- 810: "The methods analogical to GeomLib::AllowExtend(...) that can be applied to a surface are called..."
- 856: must be "of their surfaces"
- 858: We must also introduce the methods IsPeriodic(), IsUPeriodic(), IsVPeriodic(). Otherwise it is unclear from the text how to avoid calling Period() in non-periodic cases.
- 862: "the behavior of these methods"
(0075099)
msv (developer)
2018-04-02 12:16

src/Adaptor2d/Adaptor2d_Curve2d.hxx
- 89: extra "//!"
- Making the methods pure virtual is non consistent here: 1) their implementation in cxx is not removed; 2) due to behavior of all other methods.

src/Adaptor2d/Adaptor2d_HCurve2d.hxx
- 79: extra "//!"

src/Adaptor3d/Adaptor3d_Curve.hxx
- 91: extra "//!"
- Making the methods pure virtual is non consistent with behavior of all other methods.

src/Adaptor3d/Adaptor3d_Surface.hxx
- Making the methods pure virtual is non consistent with behavior of all other methods.

To be continued.
(0075109)
git (administrator)
2018-04-02 15:55

Branch CR29115_2 has been created by nbv.

SHA-1: 7a1e372e5efe0a4a9d0284aef129775fc8a8255e


Detailed log of new commits:

Author: nbv
Date: Wed Feb 21 17:31:37 2018 +0300

    0029115: [Regression] GeomAdaptor mistakenly reports non-periodic curves to be periodic
    
    1. Section "Periodicity concept" has been added in Doxigen-documentation
    
    2. Method IsPeriodic() of classes Geom(2d)_TrimmedCurve has changed its behavior. Please see the documentation about correspond methods.
    
    3. Methods IsUPeriodic() and IsVPeriodic() has changed their behavior.
    
    4. Methods Geom_SurfaceOfLinearExtrusion::UPeriod() and Geom_SurfaceOfRevolution::VPeriod() has changed their behavior.
    
    5. Method GeomLib::IsClosed(...) has been added in order to check closure of 3D/2D curve with some tolerance.
    
    6. Methods ShapeAnalysis_Curve::IsPeriodic(...), BRepBlend_HCurve2dTool::IsPeriodic(...), Contap_HCurve2dTool::IsPeriodic(...), IntPatch_HCurve2dTool::IsPeriodic(...), BRepBlend_HCurveTool::IsPeriodic(...) have been removed.
    
    7. Method
    
       void BOPTools_AlgoTools2D::AdjustPCurveOnFace (const TopoDS_Face& theF,
                                                      const Handle(Geom_Curve)& theC3D,
                                                      const Handle(Geom2d_Curve)& theC2D,
                                                      Handle(Geom2d_Curve)& theC2DA,
                                                      const Handle(IntTools_Context)& theContext = Handle(IntTools_Context)());
    
    has been removed.
    
    8. Methods GeomLib::AllowExtend(...), GeomLib::AllowExtendUParameter(...) and GeomLib::AllowExtendVParameter(...) have been created.
    
    9. Interface of Geom_ConicalSurface::Apex(...) method has been corrected insignificantly.
    
    10. Method IntTools_Tools::IsClosed(...) has been removed.
    
    11. The DRAW-commands "curveperiod" and "surfaceperiod" have been created. Please see help-system to obtain the information these commands.
    
    12. Several DRAW-commands have been created to provide work with adapters. They are defined in the file GeomliteTest_AdaptorCommands.cxx. Currently they allow reading the following adapter's properties: periodicity, closure and resolution.
    
    13. New test cases have been created.
(0075110)
nbv (developer)
2018-04-02 15:55

Dear Mikhail,

Please review CR29115_2 branch
(0075117)
msv (developer)
2018-04-02 16:52

src/GeomLib/GeomLib.hxx
- 35: double declaration of Geom_Curve
- 38: unused declaration of Adaptor3d_Surface
- 60,61: unused includes
- 201: "defines" => "determines"
- 223: why this method takes non-handle curve unlike IsClosed()? Make uniform parameters.
- The name AllowExtend is not exact. The better name is IsTrimAllowed. The same is for methods IsUTrimAllowed, IsVTrimAllowed.

src/GeomLib/GeomLib.cxx
- 2760: make protection against infinite FirstVParameter.
- 2790: make protection against infinite FirstUParameter.
- 2792: make protection against null iso curve.
- 2838: decrease zone of invalidity instead of increasing it.

To be continued.
(0075120)
git (administrator)
2018-04-02 17:56

Branch CR29115_3 has been created by nbv.

SHA-1: 9e43d8b8ada842be69593000ec64d926a9da1b33


Detailed log of new commits:

Author: nbv
Date: Wed Feb 21 17:31:37 2018 +0300

    0029115: [Regression] GeomAdaptor mistakenly reports non-periodic curves to be periodic
    
    1. Section "Periodicity concept" has been added in Doxigen-documentation
    
    2. Method IsPeriodic() of classes Geom(2d)_TrimmedCurve has changed its behavior. Please see the documentation about correspond methods.
    
    3. Methods IsUPeriodic() and IsVPeriodic() has changed their behavior.
    
    4. Methods Geom_SurfaceOfLinearExtrusion::UPeriod() and Geom_SurfaceOfRevolution::VPeriod() has changed their behavior.
    
    5. Method GeomLib::IsClosed(...) has been added in order to check closure of 3D/2D curve with some tolerance.
    
    6. Methods ShapeAnalysis_Curve::IsPeriodic(...), BRepBlend_HCurve2dTool::IsPeriodic(...), Contap_HCurve2dTool::IsPeriodic(...), IntPatch_HCurve2dTool::IsPeriodic(...), BRepBlend_HCurveTool::IsPeriodic(...) have been removed.
    
    7. Method
    
       void BOPTools_AlgoTools2D::AdjustPCurveOnFace (const TopoDS_Face& theF,
                                                      const Handle(Geom_Curve)& theC3D,
                                                      const Handle(Geom2d_Curve)& theC2D,
                                                      Handle(Geom2d_Curve)& theC2DA,
                                                      const Handle(IntTools_Context)& theContext = Handle(IntTools_Context)());
    
    has been removed.
    
    8. Methods GeomLib::AllowExtend(...), GeomLib::AllowExtendUParameter(...) and GeomLib::AllowExtendVParameter(...) have been created.
    
    9. Interface of Geom_ConicalSurface::Apex(...) method has been corrected insignificantly.
    
    10. Method IntTools_Tools::IsClosed(...) has been removed.
    
    11. The DRAW-commands "curveperiod" and "surfaceperiod" have been created. Please see help-system to obtain the information these commands.
    
    12. Several DRAW-commands have been created to provide work with adapters. They are defined in the file GeomliteTest_AdaptorCommands.cxx. Currently they allow reading the following adapter's properties: periodicity, closure and resolution.
    
    13. New test cases have been created.
(0075123)
git (administrator)
2018-04-02 18:23

Branch CR29115_3 has been updated forcibly by nbv.

SHA-1: 171d6639631ad634c40ab659910839d00342aa46
(0075136)
msv (developer)
2018-04-03 09:59

Put in upgrade guide info about changed API and behavior.
(0075137)
msv (developer)
2018-04-03 10:20

dox/user_guides/modeling_data/modeling_data.md
- 800: "any surface always has"

Put in commit message a pair of words about the main changes (that adaptors and trimmed curves periodicity is always got from the basis geometry).
(0075138)
abv (manager)
2018-04-03 11:23

Some remarks:

0. In Git commit, please describe the changes made, instead of (or in addition to) just giving reference to documentation.

1. Please avoid external references (to Widipedia, OCCT docs, etc.) in comments inside header files. Just describe clearly what the method does. For instance, method IsPeriodic() in Adaptor2d_HCurve2d.hxx could have comment like this:

"Returns True if basis curve is periodic"

The same in Adaptor2d_HCurve.hxx, Adaptor3d_Curve.hxx, Adaptor3d_HCurve.hxx, Adaptor3d_HSurface.hxx, Adaptor3d_Surface.hxx

2. In Adaptor2d_OffsetCurve.hxx, comment "This method is overridden from the basis class" is totally meaningless. Instead please describe what the method does. For instance, for IsPeriodic() this could be:

"Returns True if basis curve is periodic"

The same in Adaptor3d_CurveOnSurface.hxx, BRepAdaptor_Curve.hxx, BRepAdaptor_Surface.hxx

3. Please avoid Deutsch-style phrases like "Doxygen-documentation", "U-period", "help-system", "DRAW-commands", "template-method" etc. English spelling is without hyphen.

4. In BOPTools_AlgoTools3D.cxx: in declarations of several variables, please make each declaration separate statement, instead of having single comma-separated statement spanning over several lines. This will be easier to handle in the future, and error-prone for automatic formatter tools.

5. In BRepAdaptor_CompCurve.hxx, comment to method Period says "Returns delta between last and first parameters". This is not consistent with other places e.g. Geom_Curve::Period() which raises exception if curve is not periodic. Please ensure consistent behavior across all classes.

6. It would be more logical to have method Period() implemented in all classes where method IsPeriodic() is redefined: this would be more explicit and allow avoiding unnecessary check of periodicity that does Period() in the base class.

7. Methods ElCLib::AdjustPeriodic() and ToPeriod() are implemented on assumption that target range (first two arguments) defines a period. Now since we allow trimmed curves and adaptors to behave as periodic, even if their range could be less than a period, this should be revised. In my opinion, it will be logical to introduce method AdjustPeriodic() with additional argument to pass a period (which should be not less than target range) explicitly. Existing method can be replaced by the new one in all places, and declared as deprecated. Or at least, all places where these methods are used should be checked to ensure that passed range is always equal to actual period.

8. In BRepFill_MultiLine.cxx, lines 235-245 marked by comment "Is it really correct?" actually look very suspicious: (a) sphere is assumed to be not periodic by V, and (b) period is 2 PI, not 1 PI. Have you investigated this?

--- to be continued ---
(0075148)
git (administrator)
2018-04-03 13:45

Branch CR29115_4 has been created by nbv.

SHA-1: 6d91bb9d65d85ae9d5f3fb8cd9b7fd4dbf3dc89f


Detailed log of new commits:

Author: nbv
Date: Wed Feb 21 17:31:37 2018 +0300

    0029115: [Regression] GeomAdaptor mistakenly reports non-periodic curves to be periodic
    
    1. Adaptors and trimmed curves periodicity is always got from the basis geometry (independently of trim boundaries). Section "Periodicity concept" has been added in Doxigen-documentation.
    
    2. Method IsPeriodic() of classes Geom(2d)_TrimmedCurve has changed its behavior. Please see the documentation about correspond methods.
    
    3. Methods IsUPeriodic() and IsVPeriodic() has changed their behavior.
    
    4. Methods Geom_SurfaceOfLinearExtrusion::UPeriod() and Geom_SurfaceOfRevolution::VPeriod() has changed their behavior.
    
    5. Method GeomLib::IsClosed(...) has been added in order to check closure of 3D/2D curve with some tolerance.
    
    6. Methods ShapeAnalysis_Curve::IsPeriodic(...) has been removed.
    
    7. Method
    
       void BOPTools_AlgoTools2D::AdjustPCurveOnFace (const TopoDS_Face& theF,
                                                      const Handle(Geom_Curve)& theC3D,
                                                      const Handle(Geom2d_Curve)& theC2D,
                                                      Handle(Geom2d_Curve)& theC2DA,
                                                      const Handle(IntTools_Context)& theContext = Handle(IntTools_Context)());
    
    has been removed.
    
    8. Methods GeomLib::AllowExtend(...), GeomLib::AllowExtendUParameter(...) and GeomLib::AllowExtendVParameter(...) have been created.
    
    9. Interface of Geom_ConicalSurface::Apex(...) method has been corrected insignificantly.
    
    10. Method IntTools_Tools::IsClosed(...) has been removed.
    
    11. The DRAW-commands "curveperiod" and "surfaceperiod" have been created. Please see help-system to obtain the information these commands.
    
    12. Several DRAW-commands have been created to provide work with adapters. They are defined in the file GeomliteTest_AdaptorCommands.cxx. Currently they allow reading the following adapter's properties: periodicity, closure and resolution.
    
    13. New test cases have been created.
(0075160)
git (administrator)
2018-04-04 10:31

Branch CR29115_4 has been updated forcibly by nbv.

SHA-1: a6f2fad6fa96f4d91b0a2655688e8906b410aa12
(0075174)
git (administrator)
2018-04-04 19:32

Branch CR29115_5 has been created by nbv.

SHA-1: 19cc209a619799dc0be47c41415c6b8bfdf18692


Detailed log of new commits:

Author: nbv
Date: Wed Feb 21 17:31:37 2018 +0300

    0029115: [Regression] GeomAdaptor mistakenly reports non-periodic curves to be periodic
    
    1. Adaptors and trimmed curves periodicity is always got from the basis geometry (independently of trim boundaries). Section "Periodicity concept" has been added in Doxigen-documentation.
    
    2. Method IsPeriodic() of classes Geom(2d)_TrimmedCurve has changed its behavior. Please see the documentation about correspond methods.
    
    3. Methods IsUPeriodic() and IsVPeriodic() has changed their behavior.
    
    4. Methods Geom_SurfaceOfLinearExtrusion::UPeriod() and Geom_SurfaceOfRevolution::VPeriod() has changed their behavior.
    
    5. Method GeomLib::IsClosed(...) has been added in order to check closure of 3D/2D curve with some tolerance. At the same time, IntTools_Tools::IsClosed(...) method has been removed.
    
    6. Method ShapeAnalysis_Curve::IsPeriodic(...) has been removed.
    
    7. Method
    
       void BOPTools_AlgoTools2D::AdjustPCurveOnFace (const TopoDS_Face& theF,
                                                      const Handle(Geom_Curve)& theC3D,
                                                      const Handle(Geom2d_Curve)& theC2D,
                                                      Handle(Geom2d_Curve)& theC2DA,
                                                      const Handle(IntTools_Context)& theContext = Handle(IntTools_Context)());
    
    has been removed.
    
    8. Methods GeomLib::IsTrimAllowed(...), GeomLib::IsUTrimAllowed(...) and GeomLib::IsVTrimAllowed(...) have been created.
    
    9. Interface of Geom_ConicalSurface::Apex(...) method has been corrected insignificantly.
    
    10. The DRAW-commands "curveperiod" and "surfaceperiod" have been created. Please see help-system to obtain the information these commands.
    
    11. Several DRAW-commands have been created to provide work with adapters. They are defined in the file GeomliteTest_AdaptorCommands.cxx. Currently they allow reading the following adapter's properties: periodicity, closure and resolution.
    
    12. New test cases have been created.
(0075182)
git (administrator)
2018-04-05 12:27

Branch CR29115_5 has been updated forcibly by nbv.

SHA-1: 0fc7e7d114e86ebe97365e143e0987d14c010919
(0075192)
git (administrator)
2018-04-05 18:53

Branch CR29115_5 has been updated forcibly by nbv.

SHA-1: 5181bc771e9fdffe22f70cda1ad71b0bb455f7d1
(0075205)
git (administrator)
2018-04-06 09:56

Branch CR29115_5 has been updated forcibly by nbv.

SHA-1: 6b226e703de9b52f8a1f90d682fe62d3571806ba
(0075392)
git (administrator)
2018-04-12 10:28

Branch CR29115_5 has been updated forcibly by nbv.

SHA-1: d06060c900f7a862615cc2518e41e4c1043229cc
(0075394)
nbv (developer)
2018-04-12 10:31

Branches CR29115_5 and CR29115prod have been rebased on the current MASTER.
(0075423)
git (administrator)
2018-04-12 14:53

Branch CR29115_5 has been updated forcibly by nbv.

SHA-1: 651607e9c47e0bc48a1d6c237b3455d973d0dc46
(0075465)
git (administrator)
2018-04-16 15:59

Branch CR29115_5 has been updated forcibly by nbv.

SHA-1: 5ed5cf59f24f22244c7daefa667bd4f0d75c95c0

- Issue History
Date Modified Username Field Change
2017-09-15 12:07 Roman Lygin New Issue
2017-09-15 12:07 Roman Lygin Assigned To => msv
2017-09-15 12:39 abv Relationship added child of 0028724
2017-09-15 14:43 msv Note Added: 0070584
2017-09-15 14:43 msv Assigned To msv => bugmaster
2017-09-15 14:43 msv Status new => feedback
2017-09-15 14:43 msv Resolution open => no change required
2017-09-15 15:03 Roman Lygin Note Added: 0070588
2017-09-15 16:04 msv Note Added: 0070595
2017-09-18 17:43 Roman Lygin Note Added: 0070654
2017-09-19 10:41 nbv Note Added: 0070666
2017-09-19 10:42 nbv Note Edited: 0070666 View Revisions
2017-09-20 13:42 nbv Note Added: 0070711
2017-09-20 13:43 nbv Assigned To bugmaster => msv
2017-09-20 13:46 nbv Note Edited: 0070711 View Revisions
2017-09-20 18:18 Roman Lygin Note Added: 0070716
2017-09-21 02:08 msv Note Added: 0070722
2017-09-21 02:08 msv Status feedback => assigned
2017-09-21 02:09 msv Assigned To msv => nbv
2017-09-21 08:03 azv Note Added: 0070724
2017-09-21 09:40 msv Note Added: 0070727
2017-09-21 09:46 nbv Note Added: 0070728
2017-09-21 10:14 azv Note Added: 0070729
2017-09-21 10:24 nbv Note Added: 0070730
2017-10-09 14:22 abv Note Added: 0071309
2017-10-22 17:06 abv Priority normal => high
2017-10-22 17:06 abv Target Version 7.4.0* => 7.3.0
2017-10-27 13:40 nbv Relationship added related to 0000486
2017-12-05 17:08 msv Target Version 7.3.0 => 7.4.0*
2018-01-16 13:52 aml Relationship added related to 0029430
2018-03-02 11:02 nbv Target Version 7.4.0* => 7.3.0
2018-03-19 18:37 git Note Added: 0074668
2018-03-20 10:58 git Note Added: 0074690
2018-03-22 19:34 git Note Added: 0074778
2018-03-23 10:14 git Note Added: 0074796
2018-03-26 10:53 git Note Added: 0074909
2018-03-27 17:26 git Note Added: 0074974
2018-03-28 10:04 nbv Note Added: 0074984
2018-03-28 10:04 nbv Assigned To nbv => msv
2018-03-28 10:04 nbv Status assigned => resolved
2018-04-02 11:28 msv Note Added: 0075097
2018-04-02 12:16 msv Note Added: 0075099
2018-04-02 15:55 git Note Added: 0075109
2018-04-02 15:55 nbv Note Added: 0075110
2018-04-02 16:52 msv Note Added: 0075117
2018-04-02 17:56 git Note Added: 0075120
2018-04-02 18:23 git Note Added: 0075123
2018-04-03 09:59 msv Note Added: 0075136
2018-04-03 10:20 msv Note Added: 0075137
2018-04-03 11:23 abv Note Added: 0075138
2018-04-03 13:45 git Note Added: 0075148
2018-04-04 10:31 git Note Added: 0075160
2018-04-04 19:32 git Note Added: 0075174
2018-04-05 12:27 git Note Added: 0075182
2018-04-05 18:53 git Note Added: 0075192
2018-04-06 09:56 git Note Added: 0075205
2018-04-10 10:51 abv Target Version 7.3.0 => 7.4.0*
2018-04-12 10:28 git Note Added: 0075392
2018-04-12 10:31 nbv Note Added: 0075394
2018-04-12 14:53 git Note Added: 0075423
2018-04-16 15:59 git Note Added: 0075465


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker