DiscussionsIssue archiveOCCT:Modeling Algorithms

Archived issue #0026747

Some constructors of gp_Parab2d class contain redundant parameters

Open CASCADEOCCT:Modeling Algorithmsclosed42 public notes

Search issues

Description

According to the theory, parabola is set by its directrix (is a simple line, not a vector) and focus point. The distances from every point of the parabola to the directrix and to the focus point are equal. The distance from the directrix to the focus point is equal to two focal length or to parameter of parabola. At that, there is a rule to direct X-axis (symmetry-axis) of local coordinate system (UCS) from the directrix to the focus point. Y-axis is always parallel to the directrix. In this case, every point of parabola is satisfied to the equation (in UCS) y(x)^2 = 2*p*x, where p is parameter of parabola.

Now, let consider existing constructors:

1. gp_Parab2d(const gp_Ax2d& D, const gp_Pnt2d& F, const Standard_Boolean Sense = Standard_True);

I do not understand, what should be the result of this constructor. Directrix D is directed line (has gp_Ax2d type). Does it mean that the direction of Y-axe should correspond to this direction? If it is not and the directrix direction is excess parameter then why we do not use gp_Lin2d class for directrix (indeed, line has direction, but it is its internal value as distinct from vector). Otherwise (if Y-direction correspond to the direction of the directrix, i.e. is defined), X-direction is defined by the directrix and the focus point (see above). Then, what is the role of "Sense" parameter? I cannot answer.

2. Constructor gp_Parab2d(const gp_Ax22d& D, const gp_Pnt2d& F).

We have analogical situation here: gp_Ax22d class sets full coordinate system XOY. Additionally, we need in focal-length only (which must be laid in positive direction of the X-axis). Why do we use focus point? More over, what kind of parabola should we obtain if the focus point does not lie in the X-axis? I cannot answer.

P.S.

Results of parabola creation for some constructor seems to be wrong. See Steps To Reproduce.

Steps to reproduce

Test cases have been created and pushed to the branch.

          Parabola 1 (see Attachment 1 (PNG)):
// On the current MASTER, we have empty-parabola and exception in Debug-mode.
GCE2d_MakeParabola aPrb(gp_Ax2d(gp_Pnt2d(0.0, 3.0), gp_Dir2d(0.0, 1.0)), gp_Pnt2d(1.0, 3.0));

          Parabola 2 (see Attachment 2 (PNG)):
// Left-handed coordinate system is used (Sense = Standard_False).
// However, the parabola is oriented as when right-handed coordinate system is used.
// More over, the parabola intersects its directrix. It is good, if and only if
// the focal length is equal to zero. But aPrb.Value()->Parab2d().Focal() returns 1.5 (on the current MASTER).
GCE2d_MakeParabola aPrb(gp_Ax2d(gp_Pnt2d(0.0, 0.0), gp_Dir2d(0.0, 1.0)), gp_Pnt2d(-1.0, 3.0), Standard_False);

          Parabola 3 (see Attachment 3 (PNG)):
// Focus point lies in the directrix. Consequently, focal length is really equal to zero.
// But aPrb.Value()->Parab2d().Focal() returns 1.5 (on the current MASTER).
GCE2d_MakeParabola aPrb(gp_Ax2d(gp_Pnt2d(0.0, 0.0), gp_Dir2d(0.0, 1.0)), gp_Pnt2d(0.0, 3.0), Standard_False);

Additional information

All changes have been documented in dox/dev_guides/upgrade/upgrade.md file.

Public activity

42 archived notes

Participants are labeled by their role within this record.

01Commenter 1
Dear Mikhail,

There are two ways for fixing problem 1 in Bug description:

1. To eliminate strange constructors and to keep only three:
a) gp_Parab2d(const gp_Ax2d& theDirectrix, const Standard_Real FocalLength, const Standard_Boolean Sense).

b) gp_Parab2d(const gp_Ax22d& theDirectrix, const Standard_Real FocalLength).

c) gp_Parab2d(const gp_Ax2d& theDirectrix, const gp_Pnt2d& theFocus). X-axe is defined here by theFocus location.

2. To keep these constructors and make them well-documented. E.g. "theFocus is used here for computation of focal length only. Sometimes, its position can be changed in order to obtain correct parabola (which does not intersects with the directrix)."

What is the better way for us? TIA.
02Commenter 2
Dear Igor, your feedback is very appreciated.
03Commenter 3
In my opinion, we can remove strange constructor, I cannot think any case when that constructor will be useful.
04Commenter 4
Dear Nikolay, so please make the fix removing useless constructor.
Process it with low priority, please.
05Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Mon Apr 11 11:12:45 2016 +0300

    0026747: Some constructors of gp_Parab2d classes have not understandable interface and create wrong parabola
    
    1. Useless constructors have been deleted.
    2. New constructor gp_Parab2d(const gp_Ax2d& theDirectrix, const gp_Pnt2d& theFocus, const Standard_Boolean theSense) has been added.
    3. Documentation of gp_Parab2d class has been updated.
    
    Creation of test cases for this issue.
06Commenter 6
Dear Mikhail,

Please review CR26747 branch.
07Commenter 7
The commit comment is apparently incorrect and incomplete:
- constructor gp_Parab2d(const gp_Ax2d& D, const gp_Pnt2d& F, const Standard_Boolean Sense) was existing before the fix
- method Directrix() is changed and will return different result
08Commenter 2
Branch [archived branch] has been updated forcibly by Author.

[revision removed]
09Commenter 9
Commit message has been corrected according to Andrey's remarks.
10Commenter 2
Branch [archived branch] has been updated forcibly by Author.

[revision removed]
11Commenter 11
CR26747 has been rebased on the current MASTER
12Commenter 12
1) Replace the word 'triple' with 'coordinate system' in doxygen comments.
2) Do not use Standard_EXPORT declaration for inline methods.
13Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Mon Apr 11 11:12:45 2016 +0300

    0026747: Some constructors of gp_Parab2d classes have not understandable interface and create wrong parabola
    
    1. Useless constructors have been deleted.
    2. Value returned by gp_Parab2d::Directrix() method has been corrected according to common sense.
    3. Documentation of gp_Parab2d class has been updated (in hxx-files).
    
    Creation of test cases for this issue.
14Commenter 14
Dear Mikhail,

Please review CR26747_1 branch.
15Commenter 15
Please avoid code duplication as much as possible. For that, make only one draw function OCC26747 that will use branches inside according to the command name, and reuse the common code.
16Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Mon Apr 11 11:12:45 2016 +0300

    0026747: Some constructors of gp_Parab2d classes have not understandable interface and create wrong parabola
    
    1. Useless constructors have been deleted.
    2. Value returned by gp_Parab2d::Directrix() method has been corrected according to common sense.
    3. Documentation of gp_Parab2d class has been updated (in hxx-files).
    
    Creation of test cases for this issue.
17Commenter 17
Dear Mikhail,

Please review CR26747_2 branch.
18Commenter 18
- Please name global variables in namespace Parab2d_Bug26747 starting with capital letter. The prefix 'a' should be used for local variables only.
19Commenter 2
Branch [archived branch] has been updated forcibly by Author.

[revision removed]
20Commenter 20
Dear Mikhail,

Please review the current state of CR26747_2 branch. This branch has already been rebased on the current MASTER.
21Commenter 21
Reviewed.
22Commenter 22
Dear nbv,
could you please rebase branch CR26747_2 on IR-2016-05-26 branch, there are conflict files.
23Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Mon Apr 11 11:12:45 2016 +0300

    0026747: Some constructors of gp_Parab2d classes have not understandable interface and create wrong parabola
    
    1. Useless constructors have been deleted.
    2. Value returned by gp_Parab2d::Directrix() method has been corrected according to common sense.
    3. Documentation of gp_Parab2d class has been updated (in hxx-files).
    
    Creation of test cases for this issue.
24Commenter 24
Done.

Please test CR26747_3 branch.
25Commenter 25
Dear Commenter 1,
Branch CR26747_3 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
[revision removed]

Number of compiler warnings:

occt component :
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

products component :
Linux: 72 (72 on master)
Windows: 0 (4 on master)
MacOS : 1158

Regressions/Differences/Improvements:
No regressions/differences

Testing cases:
http://occt-tests/CR26747-3-master-OCCT/Debian70-64/bugs/modalg_6/bug26747_1.html
http://occt-tests/CR26747-3-master-OCCT/Windows-64-VC10/bugs/modalg_6/bug26747_1.html
bugs modalg_6 bug26747_1: OK
http://occt-tests/CR26747-3-master-OCCT/Debian70-64/bugs/modalg_6/bug26747_2.html
http://occt-tests/CR26747-3-master-OCCT/Windows-64-VC10/bugs/modalg_6/bug26747_2.html
bugs modalg_6 bug26747_2: OK
http://occt-tests/CR26747-3-master-OCCT/Debian70-64/bugs/modalg_6/bug26747_3.html
http://occt-tests/CR26747-3-master-OCCT/Windows-64-VC10/bugs/modalg_6/bug26747_3.html
bugs modalg_6 bug26747_3: OK

Testing on Linux:
occt component :
Total MEMORY difference: 88370541 / 88307858 [+0.07%]
Total CPU difference: 18610.110000000037 / 18553.190000000006 [+0.31%]
products component :
Total MEMORY difference: 27267555 / 27256815 [+0.04%]
Total CPU difference: 5203.3899999999785 / 5188.639999999986 [+0.28%]

Testing on Windows:
occt component :
Total MEMORY difference: 55934537 / 55926481 [+0.01%]
Total CPU difference: 18088.565551598855 / 18435.979778598783 [-1.88%]
products component :
Total MEMORY difference: 18919594 / 18883714 [+0.19%]
Total CPU difference: 5006.274891299942 / 5050.469974599949 [-0.88%]

There are no differences in images found by testdiff.
26Commenter 26
Dear Commenter 1,
Branch CR26747_3 is TESTED.
27Commenter 27
Please correct issue summary and commit message to provide meaningful description of the problem and the changes made. Avoid vague wording like "not understandable", "common sense" etc. If behavior of the API class changes (I suppose that gp_Parab2d being a data structure, is an API class), please describe that change in Upgrade Guide (dox/upgrade/upgrade.md) so that projects which use that API can recognize the change, and adapt their code accordingly.
28Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Mon Apr 11 11:12:45 2016 +0300

    0026747: Some constructors of gp_Parab2d class contain redundant parameters
    
    1. Useless constructors have been deleted.
    2. Value returned by gp_Parab2d::Directrix() method has been corrected (there is no point in reversing directrix).
    3. Documentation of gp_Parab2d class has been updated (in hxx-file).
    4. Release Notes have been updated according to corrections made in this issue.
    
    Creation of test cases for this issue.
29Commenter 29
Dear Andrei,

Please review the current state of CR26747_4 branch.
30Commenter 2
Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Mon Apr 11 11:12:45 2016 +0300

    0026747: Some constructors of gp_Parab2d class contain redundant parameters
    
    1. Useless constructors have been deleted.
    2. Value returned by gp_Parab2d::Directrix() method has been corrected to exclude reversing the directrix.
    3. Documentation of gp_Parab2d class has been updated (in hxx-file).
    4. Upgrade Guide has been updated according to corrections made in this issue.
    
    Creation of test cases for this issue.
31Commenter 31
Branch CR26747_5 is reviewed, please consider as tested (there are no differences in code from CR26747_3)
32Commenter 32
Description has been updated.
33Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
34Commenter 34
Dear Commenter 1,
Branch CR26747_5 was rebased on current master of occt git-repository.
[revision removed]
35Commenter 35
Dear Commenter 1,
Branch CR26747_5 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
[revision removed]

Number of compiler warnings:

occt component :
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

products component :
Linux: 72 (72 on master)
Windows: 4 (4 on master)
MacOS : 1148
36Commenter 36
Dear Commenter 1,
Branch CR26747_5 is TESTED.
37Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]
38Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]
39Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]
40Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]
41Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]
42Commenter 2
Branch [archived branch] has been deleted by Participant.

[revision removed]

Related records