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

Geometry_Engine: Adding methods required by IElement2D for PlanarSurface #2835

Merged
merged 5 commits into from
May 27, 2022

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh commented May 19, 2022

NOTE: Depends on

BHoM/BHoM#1390

Issues addressed by this PR

Closes #2812

Adding methods required for IElement2D.

For planar surfaces, the InternalElement2Ds will be returned as a set of new PLanarSurfaces, with ExternalBoundaries correpsonding to the InternalBoundaries of the host PlanarSurface. This is to fullfill the requirement that the openings must also be IElement2D, which is not the case for the closed curves. This workaround alows for the methods to behave as expected.

Potential at one point to add some form of Region class, that is like the PlanarSurface, but without any openings. There is no practical use for that class though, that can not be achieved with a PlanarSurface with no openings, more than clarity for methods like this. For now, I think this is the best solution forward, even it it at first might look slightly unintuitive.

Test files

https://burohappold.sharepoint.com/:f:/s/BHoM/EgJ3TMjsnNFBkM_sPjRKDewBcjco6-8OogC7UcSvS3lMKw?e=g8munn

Changelog

Additional comments

Reference to the wikipage outlining methods required by the IElement framework:

https://github.com/BHoM/documentation/wiki/IElement-required-extension-methods

@IsakNaslundBh IsakNaslundBh self-assigned this May 20, 2022
@IsakNaslundBh IsakNaslundBh added the type:feature New capability or enhancement label May 20, 2022
@IsakNaslundBh IsakNaslundBh marked this pull request as ready for review May 20, 2022 12:55
@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented May 20, 2022

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

  • code-compliance
  • documentation-compliance
  • project-compliance
  • branch-compliance
  • dataset-compliance
  • copyright-compliance

There are 561 requests in the queue ahead of you.

Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Good in general, see comments

Co-authored-by: Alessio Lombardi <alessio.lombardi@burohappold.com>
alelom
alelom previously approved these changes May 27, 2022
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Tested using test scripts, approving based on latest changes and discussion in comments

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance

@bhombot-ci
Copy link

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

There are 130 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented May 27, 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 @IsakNaslundBh on BHoM

@bhombot-ci
Copy link

bhombot-ci bot commented May 27, 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 @IsakNaslundBh on BHoM

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check null-handling

@bhombot-ci
Copy link

bhombot-ci bot commented May 27, 2022

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

  • null-handling

There are 127 requests in the queue ahead of you.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance

@bhombot-ci
Copy link

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

There are 126 requests in the queue ahead of you.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented May 27, 2022

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

  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • versioning
  • installer

There are 135 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented May 27, 2022

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented May 27, 2022

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented May 27, 2022

The check null-handling has already been run previously and recorded as a successful check. This check has not been run again at this time.

@alelom alelom self-requested a review May 27, 2022 15:29
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Re-approving after null handling fixes

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented May 27, 2022

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

  • ready-to-merge

There are 118 requests in the queue ahead of you.

@IsakNaslundBh IsakNaslundBh merged commit 6d6fd16 into main May 27, 2022
@IsakNaslundBh IsakNaslundBh deleted the Geometry_oM-#426-MakePlanarSurfaceIElement2D branch May 27, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geometry_Engine: Add required methods for IElement2D for planar surface
2 participants