Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IfcCosine #561

Merged
merged 4 commits into from
Aug 18, 2022
Merged

IfcCosine #561

merged 4 commits into from
Aug 18, 2022

Conversation

Elvira2227
Copy link
Collaborator

@Elvira2227 Elvira2227 commented Jul 29, 2022

In this pull request adds new implementation for IfcCosine.
The sturcture of the functions getpointOnCurve and getDirectionOnCurve are similar with representation of IfcSpline.

Note:
There are a mistake in allready merged functions of IfcSine, for instance in the declaration of the template functions getPointOnCurve and getDirectionOfCurve. Previously, the code did not reached this functions and it represents geometry only with cartesianPointList #565 . Now this mistake has been fixed.
Another point corresponds to issue #560. The calculation takes quite a long time.

In the example file cosine-curve_100.0_300_1000_1_Meter were are not 100 point as usual, but 101. Therefore, our implementation does not reach the last point. I think, it is a bug in example file.
Here is a screenshot of this example. And we see that the maximum point, which should be the last point, is just the penultimate point.
image

ToDo:

  • Add contexte to ReleaseNotes
  • Add context to SupportedIFCrepresentations

@Elvira2227 Elvira2227 requested review from pjanck and jschlenger July 29, 2022 13:18
@Elvira2227 Elvira2227 self-assigned this Jul 29, 2022
@Elvira2227 Elvira2227 added the IFC Content related to Industry Foundation Classes (IFC) functionalities label Jul 29, 2022
Copy link
Collaborator

@jschlenger jschlenger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far.
Did you already work on the issue with the long calculation times?

Copy link
Contributor

@pjanck pjanck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lftm

Is there an issue/PR for the unit tests?

@pjanck pjanck added the example-files Issue related to failing reading an example file label Aug 11, 2022
@jschlenger jschlenger merged commit 70358dd into tumcms:development Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
example-files Issue related to failing reading an example file IFC Content related to Industry Foundation Classes (IFC) functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants