Archived issue #0026747
Some constructors of gp_Parab2d class contain redundant parameters
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.
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);
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.
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.
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.
Dear Igor, your feedback is very appreciated.
In my opinion, we can remove strange constructor, I cannot think any case when that constructor will be useful.
Dear Nikolay, so please make the fix removing useless constructor.
Process it with low priority, please.
Process it with low priority, please.
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.
[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.
Dear Mikhail,
Please review CR26747 branch.
Please review CR26747 branch.
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
- 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
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
Commit message has been corrected according to Andrey's remarks.
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
CR26747 has been rebased on the current MASTER
1) Replace the word 'triple' with 'coordinate system' in doxygen comments.
2) Do not use Standard_EXPORT declaration for inline methods.
2) Do not use Standard_EXPORT declaration for inline methods.
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.
[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.
Dear Mikhail,
Please review CR26747_1 branch.
Please review CR26747_1 branch.
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.
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.
[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.
Dear Mikhail,
Please review CR26747_2 branch.
Please review CR26747_2 branch.
- Please name global variables in namespace Parab2d_Bug26747 starting with capital letter. The prefix 'a' should be used for local variables only.
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
Dear Mikhail,
Please review the current state of CR26747_2 branch. This branch has already been rebased on the current MASTER.
Please review the current state of CR26747_2 branch. This branch has already been rebased on the current MASTER.
Reviewed.
Dear nbv,
could you please rebase branch CR26747_2 on IR-2016-05-26 branch, there are conflict files.
could you please rebase branch CR26747_2 on IR-2016-05-26 branch, there are conflict files.
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.
[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.
Done.
Please test CR26747_3 branch.
Please test CR26747_3 branch.
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.
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.
Dear Commenter 1,
Branch CR26747_3 is TESTED.
Branch CR26747_3 is TESTED.
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.
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.
[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.
Dear Andrei,
Please review the current state of CR26747_4 branch.
Please review the current state of CR26747_4 branch.
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.
[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.
Branch CR26747_5 is reviewed, please consider as tested (there are no differences in code from CR26747_3)
Description has been updated.
Branch [archived branch] has been updated forcibly by Participant.
[revision removed]
[revision removed]
Dear Commenter 1,
Branch CR26747_5 was rebased on current master of occt git-repository.
[revision removed]
Branch CR26747_5 was rebased on current master of occt git-repository.
[revision removed]
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
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
Dear Commenter 1,
Branch CR26747_5 is TESTED.
Branch CR26747_5 is TESTED.
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Related records