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.

Sign up for GitHub

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

Jump to bottom

functions getDirectionOnCurve for Circle and Ellipse #494

Merged
merged 19 commits into from
Jan 27, 2022
Merged

functions getDirectionOnCurve for Circle and Ellipse #494

merged 19 commits into from
Jan 27, 2022

Conversation

SamuilsRulovs
Copy link
Collaborator

@SamuilsRulovs SamuilsRulovs commented Dec 3, 2021 •
edited
Loading

Fixes #495

Functions

  • getAngleOnCircle
  • getAngleOnEllipse
  • getDirectionOnCurve for Circle
  • getDirectionOnCurve for Ellipse

@SamuilsRulovs SamuilsRulovs added IFC Content related to Industry Foundation Classes (IFC) functionalities refactoring Code refactoring related task labels Dec 3, 2021
@SamuilsRulovs SamuilsRulovs self-assigned this Dec 3, 2021
@SamuilsRulovs SamuilsRulovs requested a review from pjanck December 10, 2021 10:23
@SamuilsRulovs SamuilsRulovs marked this pull request as ready for review December 10, 2021 10:23
@SamuilsRulovs SamuilsRulovs changed the title [WIP] functions getDirectionOnCurve for Circle and Ellipse functions getDirectionOnCurve for Circle and Ellipse Dec 13, 2021
pjanck
pjanck suggested changes Dec 14, 2021
View reviewed changes
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.

This definitely goes in the correct direction (pun intended)!

pjanck
pjanck reviewed Jan 5, 2022
View reviewed changes
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.

Do you need any support?

pjanck
pjanck reviewed Jan 23, 2022
View reviewed changes
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.

Question: is the tangent calculated as going clock-wise or counter-clock-wise for the circle and ellipse?

@SamuilsRulovs
Copy link
Collaborator Author

SamuilsRulovs commented Jan 23, 2022 •
edited
Loading

Question: is the tangent calculated as going clock-wise or counter-clock-wise for the circle and ellipse?

I guess it is going counter-clock-wise since for circle
For ellipse it should be another solution, as I understand now, but I will writing it also for counter-clock-wise variant
(Right now I am trying to understand, how to implement tangent calculation for ellipse)

@pjanck
Copy link
Contributor

pjanck commented Jan 23, 2022

Right now I am trying to understand, how to implement tangent calculation for ellipse

Perhaps this might help you?

pjanck
pjanck reviewed Jan 24, 2022
View reviewed changes
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.

Is there anything else planned for this PR?

@SamuilsRulovs
Copy link
Collaborator Author

SamuilsRulovs commented Jan 27, 2022 •
edited
Loading

I guess this PR is ready, I am not planning anything else for this PR. Unless I should already refactor IfcCircle and IfcEllipse functions accordingly

@SamuilsRulovs SamuilsRulovs requested a review from pjanck January 27, 2022 10:57
pjanck
pjanck suggested changes Jan 27, 2022
View reviewed changes
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.

One function takes ellipse->Position into account, the other doesn't. Intentional?

Suggestion (DRY): Calculate angle from the CartesianPoint and call the other overload.

@pjanck
Copy link
Contributor

pjanck commented Jan 27, 2022

Unless I should already refactor IfcCircle and IfcEllipse functions accordingly

New PR, please.

@SamuilsRulovs
Copy link
Collaborator Author

SamuilsRulovs commented Jan 27, 2022 •
edited
Loading

One function takes ellipse->Position into account, the other doesn't. Intentional?

No it wasn't intentional, I guess both functions should use ellipse->Position. But I will do as you suggested and use second function to make both calculations.

@SamuilsRulovs SamuilsRulovs requested a review from pjanck January 27, 2022 11:25
@SamuilsRulovs
Copy link
Collaborator Author

I guess with ad45e76 PR is ready to be merged

@SamuilsRulovs SamuilsRulovs requested a review from pjanck January 27, 2022 11:51
@jschlenger
Copy link
Collaborator

Looks fine to me.
However I didn't check for mathematical correctness of all calculations.
But they seemed to make sense.

@pjanck pjanck merged commit 7ee6882 into tumcms:development Jan 27, 2022
@SamuilsRulovs SamuilsRulovs deleted the getDirectionOnCurve branch February 4, 2022 15:29
@Elvira2227 Elvira2227 mentioned this pull request Feb 8, 2022
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjanck pjanck pjanck approved these changes

@jschlenger jschlenger jschlenger approved these changes

Assignees

@SamuilsRulovs SamuilsRulovs

Labels
IFC Content related to Industry Foundation Classes (IFC) functionalities refactoring Code refactoring related task
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[REF] Create functions getDirectionOnCurve for Circle and Ellipse
3 participants
@SamuilsRulovs @pjanck @jschlenger