IfcPolynomialCurve #539

Elvira2227 · 2022-04-16T12:23:07Z

In this pull request implementation was added for IfcPolynomialCurve.
The following functions were added in CurveConverter.h:

getPointOnCurve
getDirectionOfCurve - here we need to implement additional code in covertIfcCurveSegment to calculate rotation around x,y axis and also rotation in 3D. Now we can only rotate around axis z.
calculatePolynomialCurve - to calculate polynomial from coefficietnts

For explanation there is a screen shot of cubic curve with coefficientsX (0., 1., 0.) and coefficientsY (0., 0., 0.01, 0.).

Note:
This is related to #524
Please see starting from commit 2b83b5b

…n helmert curve

pjanck

General remarks:

some unit tests are still missing a screen shot
it seems weird to have so many unit tests for line that seem all equal
some png are not needed as they are not used in checking

pjanck · 2022-04-17T09:37:50Z

...mas/IFC4X3_RC4/bloss-curve_100.0_300_1000_1_Meter/src/bloss-curve_100.0_300_1000_1_Meter.cpp

+	buw::storeImage(testPath("bloss-curve_100.0_300_1000_1_Meter.png").string(), image);
+
+	// Assert
+	EXPECT_NO_THROW(buw::loadImage4b(testPath("bloss-curve_100.0_300_1000_1_Meter.png").string()));


I can't seem to see this file commited.

pjanck · 2022-04-17T09:39:18Z

...Schemas/IFC4X3_RC4/clothoid_100.0_1000_-300_1_Meter/src/clothoid_100.0_1000_-300_1_Meter.cpp

+	buw::Image4b image = rendererIfc->captureImage();
+
+	// Act
+	buw::storeImage(testPath("clothoid_100.0_1000_-300_1_Meter.png").string(), image);


The file with this name does not need be committed (only the top view is needed for these unit tests). Apply for all unit tests.

jschlenger

I'm really not sure if your angle calculation is right.
Please double check and test with break points if the produced values make sense.

jschlenger · 2022-05-10T09:30:33Z

Core/src/IfcGeometryConverter/CurveConverter.h

+					if (polynomialConstantCntX>0 && polynomialConstantCntY>0)
+					{
+						double angle = std::atan2(angleY, angleX);
+						return carve::geom::VECTOR(std::cos(angle), std::sin(angle), 0.);
+
+					}


Are you sure that you need to insert angleX and angleY here?
From a quick view it would feel more natural to insert pointX and pointY into atan to get an angle than using two angles to get another angle.
Please take a closer look here to see if what you've done produces the results that you expect.

I think you are right.
I thought that we can find separatelly the angles for X and Y polynomial. Thats why I used the same function from SpiralUtils as for the clothoids.
My suggestion to use the implementation from this sourse : https://firebirdsql.org/refdocs/langrefupd21-intfunc-atan2.html
Then we can use atan2(sin(angleY), cos(angleX)).
See cf59b36

I have tasted different cases and this is a result that presents the first data of vector segmentDirections

angle = atan2(std::sin(angleY),std::cos(angleX));
carve::geom::vector<3> v1 = carve::geom::VECTOR(std::cos(angle), std::sin(angle), 0. );

[0]:{1.0000000000000000, 0.0000000000000000, 0.0000000000000000}- corresponds to point (0,0,0)
[1]:{0.93699400295766744, 0.34934544282324143, 0.0000000000000000}
[2]:{-0.67306627244558215, 0.73958217453925257, 0.0000000000000000}
...
angle = atan2(y,x);
carve::geom::vector<3> v2 = carve::geom::VECTOR(std::cos(angle), std::sin(angle), 0. );

[0]:{1.0000000000000000, 0.0000000000000000, 0.0000000000000000} - corresponds to point (0,0,0)
[1]:{0.78609999999999047, 0.61795320999998504, 0.0000000000000000}
[2]: {1.2495999999999947, 1.5615001599999867, 0.0000000000000000}
...

It only says that we have similar values for different calculations. Interestingly, our rotation matrix uses only the first point (segmentDirections[0]) to detect the direction. I think this should be the next task to check over.

P.S: I decided to place your suggestion about angle calculations.

Core/src/IfcGeometryConverter/CurveConverter.h

Elvira2227 · 2022-05-12T15:46:20Z

There are several changes in the last commits cf59b36 and 28f6421.

First, I was trying to find corresponding functions from the SpiralUtils, but it returned high values of X and Y. Thats why I desided to write a separate functions which calculates integral by using numerical methods. That will helps to find appropriate values for X and Y.
New functions integrateParameter , calculatePolynomialDerivative, intagral have been placed in CurveConverter.h.
I have noticed that integrateParameter returned the value with the approximation error 0.4%.
In the commit 28f6421 are small corrections for integrateParameter function.
I tested my code with polynomialConstantX = (0.,1.,0.) and polynomialConstantY = (0.,0.,1., 0.)

Here is the implemented polycurve:

pjanck

Some suggestions :)

Core/src/IfcGeometryConverter/CurveConverter.h

pjanck

Need to review the code more thoroughly, just one quick comment.

Core/src/IfcGeometryConverter/CurveConverter.h

jschlenger

A couple of minor comments here and there

Core/src/IfcGeometryConverter/CurveConverter.h

jschlenger · 2022-06-02T09:30:56Z

Core/src/IfcGeometryConverter/CurveConverter.h

+					// calculate angle between two polynomial curves
+					//double angle = std::atan2(angleY, angleX);
+
+					if (polynomialConstantCntX>0 && polynomialConstantCntY>0)


Is the polynomialConstantCnt only used for these if clauses?
If that's the case one could save a couple of lines by just writing
if (polynomialCurve->CoefficientsX && polynomialCurve->CoefficientsY)
then the polynomialConstCnt could be removed completely
However, this is just a personal preference and can also be left the way it is right now

I use polynomialConstantCntXYZ to check if the coefficients exist.
Because in the beginning, I create polynomialConstantXYZ vectors and then check it in this if statement it should always return true.
If polynomialConstantXYZ exist and its size polynomialConstantCntXYZ return positive value.

Core/src/IfcGeometryConverter/CurveConverter.h

jschlenger · 2022-06-02T09:39:17Z

Core/src/IfcGeometryConverter/CurveConverter.h

+					if (polynomialCurve->CoefficientsX)
+					{
+						// Interpret coefficients for X
+						coefficientsX = polynomialCurve->CoefficientsX;
+						// convert to double
+						polynomialConstantX.resize(coefficientsX.size());
+						std::transform(coefficientsX.begin(), coefficientsX.end(), polynomialConstantX.begin(), [](auto it) { return it; });
+						polynomialConstantCntX = std::size(polynomialConstantX);
+						//x = SpiralUtils::XbyAngleDeviationPolynomial(polynomialConstantX, polynomialConstantCntX, parameter);
+						//x = SpiralUtils::XbyAngleDeviationPolynomialByTerms( 0., 0., 0., 0., 0., 0., 1., 0., parameter);
+
+					}


Could it make sense to put this coefficient x y z retrieval into a separate function that is then called by getPointOnCurve and by getDirectionOfCurve?
Looks like the same code is used at least twice which could be avoided

Core/src/IfcGeometryConverter/CurveConverter.h

…urve

…ynomialCurve" This reverts commit 575861e, reversing changes made to 963fd76.

…o IfcPolynomialCurve"" This reverts commit abde8e1.

pjanck

Not there yet, right?

Elvira2227 added 11 commits October 14, 2021 12:38

New unit tests was added

70fb029

Merge remote-tracking branch 'tumcms/development' into UTIFC4X3_RC4

259e1ae

Merge branch 'development' into UTIFC4X3_RC4

aed79c3

Merge remote-tracking branch 'tumcms/development' into UTIFC4X3_RC4

1ae9e23

Added new UT for clothoid's and lines, images were updated

ea463d4

Updated screenshots and unit tests; Fixed problem with reading .ifc i…

71f684b

…n helmert curve

Updated some UTs; Circular arc screenshots updated and fixed mistake

5adcd71

Cubic ifc Datei updated

4089f6d

Merge remote-tracking branch 'tumcms/development' into UTIFC4X3_RC4

a0e7d1d

Merge remote-tracking branch 'tumcms/development' into UTIFC4X3_RC4

5ab97b8

new functions for IfcPolynomial Curve was added

2b83b5b

Elvira2227 requested review from jschlenger and pjanck April 16, 2022 12:23

Elvira2227 self-assigned this Apr 16, 2022

Elvira2227 added enhancement New feature or request IFC Content related to Industry Foundation Classes (IFC) functionalities labels Apr 16, 2022

pjanck reviewed Apr 17, 2022

View reviewed changes

jschlenger reviewed May 10, 2022

View reviewed changes

Functions to calculate integral value for parametric curve

cf59b36

Elvira2227 requested review from jschlenger and pjanck May 12, 2022 15:47

corrections for integrateParameter function

28f6421

pjanck reviewed May 15, 2022

View reviewed changes

Core/src/IfcGeometryConverter/CurveConverter.h Outdated Show resolved Hide resolved

Core/src/IfcGeometryConverter/CurveConverter.h Outdated Show resolved Hide resolved

improve code; add functions for 3D; correct mistakes

98adeb1

Elvira2227 requested a review from pjanck May 27, 2022 16:00

pjanck reviewed May 28, 2022

View reviewed changes

Core/src/IfcGeometryConverter/CurveConverter.h Outdated Show resolved Hide resolved

correct mistake (delete fstream)

aac48ed

Elvira2227 requested a review from pjanck May 29, 2022 11:11

jschlenger reviewed Jun 2, 2022

View reviewed changes

Elvira2227 added 5 commits June 10, 2022 14:59

Improve calculatePolynomialDerivative function; fixing mistakes

102578f

Removing curly braces

963fd76

Merge remote-tracking branch 'tumcms/development' into IfcPolynomialC…

575861e

…urve

Revert "Merge remote-tracking branch 'tumcms/development' into IfcPol…

abde8e1

…ynomialCurve" This reverts commit 575861e, reversing changes made to 963fd76.

Revert "Revert "Merge remote-tracking branch 'tumcms/development' int…

90056c7

…o IfcPolynomialCurve"" This reverts commit abde8e1.

Elvira2227 mentioned this pull request Jun 13, 2022

[REQUEST]Implementing corresponding functions for IfcSpline #524

Open

6 tasks

pjanck reviewed Jun 14, 2022

View reviewed changes

IfcPolynomialCurve #539

Are you sure you want to change the base?

IfcPolynomialCurve #539

Uh oh!

Conversation

Elvira2227 commented Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pjanck left a comment

Choose a reason for hiding this comment

Uh oh!

pjanck Apr 17, 2022

Choose a reason for hiding this comment

Uh oh!

pjanck Apr 17, 2022

Choose a reason for hiding this comment

Uh oh!

jschlenger left a comment

Choose a reason for hiding this comment

Uh oh!

jschlenger May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Elvira2227 May 12, 2022

Choose a reason for hiding this comment

Uh oh!

Elvira2227 May 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Elvira2227 commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pjanck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pjanck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jschlenger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jschlenger Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

Elvira2227 Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jschlenger Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pjanck left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Elvira2227 commented Apr 16, 2022 •

edited

Loading

jschlenger May 10, 2022 •

edited

Loading

Elvira2227 commented May 12, 2022 •

edited

Loading