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

Geometry_Engine: Support closed planar curve types #2841

Merged

IsakNaslundBh merged 18 commits into main from Geometry_oM-#1392-AddSupportClosedPlanarCurveTypes

Sep 29, 2022

Contributor

IsakNaslundBh commented May 23, 2022 •

edited by alelom

Loading

NOTE: Depends on

Issues addressed by this PR

Closes #2803

Adds support for the new Polygon and BoundaryCurve curves.

Change the signature of some of the more heavy methods to accept IPolyline and IPolyCurve instead of Polyline and PolyCurve, to enable passing Polygons and BoundaryCurves to them instead.

Update Centroid method for lists of curves to avoid calling BooleanUnion if it can be avoided.

Test files

https://burohappold.sharepoint.com/:f:/s/BHoM/EkuLDyGrA2FHi4MlPS1N-WMBCcfi8sdjzIigKDsL1FuHOA?e=XAFq1E

Changelog

Additional comments

IsakNaslundBh added 5 commits

May 23, 2022 15:33


          Create methods for Polygon and Polycurve

a40e335


          Adding IsClosed, IsPlanar and IsSelfIntersecting methods for Polygon …

04549ab

…and BoundaryCurve

Also, significant amount of missing descriptions added


          Adding methods for ControlPoints and Vertices

f6f203a


          Adding method for extracting the inner curves from an IPolyCurve

80c38cf


          Update Centroid methods to use the interface rather than class

acb540f

IsakNaslundBh self-assigned this

IsakNaslundBh added the type:feature label

IsakNaslundBh added 3 commits

May 24, 2022 14:58


          Methods for Area, DiscontinuityPoints, IsCLockwise and Normal to oper…

7d24648

…ate on the new interfaces


          Tweak inner working of Centroid method for two lists of curves

d514e85

Following changes done to avoid unneccesary calls to BooleanUnion, as that changes the curve type to PolyCurve, meaning and performance gain from using BoundaryCurve is lost.

Logic used for outer and inner curves is:
- If one curve, just use centroid and area from it
- If more than one curve, cluster (dbscan) by overlapping boundingboxes. For each cluster, then if single curve, just use it, if multiple curve, use boolean union, and then loop through the curve.


          Update creation of PlanarSurface to try to get curves of IBoundary

6fc3142

alelom self-requested a review

May 27, 2022 08:53

IsakNaslundBh marked this pull request as ready for review

May 27, 2022 08:56

IsakNaslundBh requested review from pawelbaran, al-fisher and epignatelli as code owners

May 27, 2022 08:56


          Minor tweak to TryGetBoundaryCurve

013dda3

Contributor Author

IsakNaslundBh commented May 27, 2022

@BHoMBot check documentation-compliance

bhombot-ci bot commented May 27, 2022

@IsakNaslundBh to confirm, the following checks are now queued:

documentation-compliance

There are 121 requests in the queue ahead of you.

IsakNaslundBh added 4 commits

May 27, 2022 16:18


          ControlPoints descriptions

633e70f


          Adding desciptions for Discontinuity points

fc56a5d

Also, adding method for Ellipse returning empty list


          IsCLockwise descriptions

01f3335


          Minor description updates

7ab3fa6

Contributor Author

IsakNaslundBh commented May 27, 2022

@BHoMBot check documentation-compliance

bhombot-ci bot commented May 27, 2022

@IsakNaslundBh to confirm, the following checks are now queued:

documentation-compliance

There are 117 requests in the queue ahead of you.


          fix input wrong name

af0391a

Contributor Author

IsakNaslundBh commented May 27, 2022

@BHoMBot check documentation-compliance

bhombot-ci bot commented May 27, 2022

@IsakNaslundBh to confirm, the following checks are now queued:

documentation-compliance

There are 116 requests in the queue ahead of you.

Contributor Author

IsakNaslundBh commented May 27, 2022

@BHoMBot check compliance
@BHoMBot check null-handling

bhombot-ci bot commented May 27, 2022

@IsakNaslundBh to confirm, the following checks are now queued:

code-compliance
documentation-compliance
project-compliance
branch-compliance
dataset-compliance
copyright-compliance
null-handling

There are 116 requests in the queue ahead of you.

alelom reviewed

View reviewed changes

Geometry_Engine/Create/BoundaryCurve.cs Outdated Show resolved Hide resolved


          null handling

a6abd33

Contributor Author

IsakNaslundBh commented May 30, 2022

@BHoMBot check null-handling

bhombot-ci bot commented May 30, 2022

@IsakNaslundBh to confirm, the following checks are now queued:

null-handling

There are 110 requests in the queue ahead of you.

Contributor

FraserGreenroyd commented Aug 3, 2022

@BHoMBot check unit-tests

bhombot-ci bot commented Aug 3, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

check unit-tests

Contributor

FraserGreenroyd commented Aug 3, 2022

@BHoMBot check unit-tests

bhombot-ci bot commented Aug 3, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

check unit-tests

Contributor

FraserGreenroyd commented Aug 3, 2022

@BHoMBot check unit-tests

bhombot-ci bot commented Aug 3, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

check unit-tests

There are 1 requests in the queue ahead of you.

Contributor

FraserGreenroyd commented Aug 3, 2022

@BHoMBot check unit-tests

bhombot-ci bot commented Aug 3, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

check unit-tests

There are 2 requests in the queue ahead of you.

Contributor

FraserGreenroyd commented Aug 3, 2022

@BHoMBot check unit-tests

bhombot-ci bot commented Aug 3, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

check unit-tests

There are 6 requests in the queue ahead of you.

IsakNaslundBh added 2 commits

September 26, 2022 09:01


          Merge branch 'main' into pr/2841

508244a


          Updating previous version attributes

alelom self-requested a review

September 26, 2022 10:15

alelom approved these changes

View reviewed changes

Member

alelom left a comment

Approving based on:

Latest code changes
Test scripts.

I am getting the same results of my previous review and I am satisfied with the answers provided in #2841 (comment), in particular on the precision "fail". The visualisation issue is still there but I am convinced it must be something on my end and not related to this PR.

bhombot-ci bot commented Sep 28, 2022

@IsakNaslundBh just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @FraserGreenroyd on BHoM

bhombot-ci bot commented Sep 28, 2022

@IsakNaslundBh just to let you know, I have provided a check-installer result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @FraserGreenroyd on BHoM

bhombot-ci bot commented Sep 28, 2022

@IsakNaslundBh just to let you know, I have provided a check-installer result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @FraserGreenroyd on OpenStreetMap_Toolkit

Contributor Author

IsakNaslundBh commented Sep 29, 2022

@BHoMBot check code-compliance
@BHoMBot check copyright-compliance
@BHoMBot check core
@BHoMBot check dataset-compliance
@BHoMBot check documentation-compliance
@BHoMBot check null-handling
@BHoMBot check project-compliance
@BHoMBot check ready-to-merge
@BHoMBot check serialisation
@BHoMBot check unit-tests

bhombot-ci bot commented Sep 29, 2022

@IsakNaslundBh to confirm, the following actions are now queued:

check code-compliance
check copyright-compliance
check core
check dataset-compliance
check documentation-compliance
check null-handling
check project-compliance
check ready-to-merge
check serialisation
check unit-tests

Contributor Author

IsakNaslundBh commented Sep 29, 2022 •

edited

Loading

The descriptions missed and flagged by the documentation compliance have intentionally been left out due to #2840 . Descriptions to be added when the method is fixed. Will dispense the check for this PR, which has been agreed in call with @FraserGreenroyd .

bhombot-ci bot commented Sep 29, 2022

FAO: @FraserGreenroyd @IsakNaslundBh is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is documentation-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 8612058397

Contributor Author

IsakNaslundBh commented Sep 29, 2022

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 8612058397

1 similar comment

Contributor Author

IsakNaslundBh commented Sep 29, 2022

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 8612058397

bhombot-ci bot commented Sep 29, 2022

@IsakNaslundBh I have now provided a passing check on reference 8612058397 as requested.

IsakNaslundBh merged commit 52945b4 into main

IsakNaslundBh deleted the Geometry_oM-#1392-AddSupportClosedPlanarCurveTypes branch

September 29, 2022 10:24

IsakNaslundBh mentioned this pull request

An error occurred on a unit test BHoM/TriangleNet_Toolkit#99

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

alelom alelom approved these changes

pawelbaran Awaiting requested review from pawelbaran pawelbaran is a code owner

al-fisher Awaiting requested review from al-fisher al-fisher is a code owner

epignatelli Awaiting requested review from epignatelli

Labels