Architecture_Engine: Geometry() query for BuildersWork.Opening refactored to return Point

[pawelbaran]

Issues addressed by this PR

Closes #2704

Test files

It is a simple property return, so no tests should be needed.

Changelog

Additional comments

This is a breaking change (different return type compared to the original version of the method) - this cannot be avoided taken the nature of the bug (Geometry() has to return Point for all types that implement IElement0D interface). Luckily, I do not think this type has been heavily used so far (if at all), taken it does not provide any interop features atm.

@IsakNaslundBh please shout if you are concerned about it and you have ideas how to lower the risk I introduce with this change.

[IsakNaslundBh]

@IsakNaslundBh please shout if you are concerned about it and you have ideas how to lower the risk I introduce with this change.

Not overly concerned by a change as long as you have checked through all use cases of the class. More wondering then about why an opening is an IElement0D in the first place?

[pawelbaran]

More wondering then about why an opening is an IElement0D in the first place?

Thanks @IsakNaslundBh. That is the question I was asking myself when working on the original oM PR: BHoM/BHoM#1258. The main rationale behind implementing IElement0D interface was that Opening is based on Cartesian coordinate system, so the most simplistic (and sufficient for now) approach is to treat it as point-based.

One could argue that it could also be 1D (taken its depth into account) or 2D (as windows/doors etc.). However, in the cases I can foresee now, there is no need for more than a point location from the code perspective, while turning Opening into IElement2D would lead to issues related with extraction of the shape and dimensions (which is usually what you need on Push).

[IsakNaslundBh]

More wondering then about why an opening is an IElement0D in the first place?

Thanks @IsakNaslundBh. That is the question I was asking myself when working on the original oM PR: BHoM/BHoM#1258. The main rationale behind implementing IElement0D interface was that Opening is based on Cartesian coordinate system, so the most simplistic (and sufficient for now) approach is to treat it as point-based.

One could argue that it could also be 1D (taken its depth into account) or 2D (as windows/doors etc.). However, in the cases I can foresee now, there is no need for more than a point location from the code perspective, while turning Opening into IElement2D would lead to issues related with extraction of the shape and dimensions (which is usually what you need on Push).

Yeah, thinking a bit more about it myself, it is quite analogous to the Bar class (a curve element with a swept profile) so can sort of understand the ifs and whys of it.

Then, I think the change is fine if you have checked through that it does not break anything (as mentioned before) and if it can be tested by someone who has used the class.

[pawelbaran]

Then, I think the change is fine if you have checked through that it does not break anything (as mentioned before) and if it can be tested by someone who has used the class.

Yeah, agreed - will make sure this gets verified before merging. This PR is a link in a larger chain of PRs to be raised today (still polishing the code), so more info, test files etc. will be added. Thanks!

[bhombot-ci]

@pawelbaran 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 @pawelbaran on Revit_Toolkit

[bhombot-ci]

@pawelbaran 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 @pawelbaran on Revit_Toolkit

[bhombot-ci]

@pawelbaran 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 @pawelbaran on Revit_Toolkit

[bhombot-ci]

@pawelbaran 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 @pawelbaran on Revit_Toolkit

[bhombot-ci]

@pawelbaran 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 @pawelbaran on Revit_Toolkit

[bhombot-ci]

@pawelbaran 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 @pawelbaran on Revit_Toolkit

[michal-pekacki]

LGTM 👍

[pawelbaran]

@BHoMBot check compliance
@BHoMBot check null-handling

[bhombot-ci]

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

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

There are 11 requests in the queue ahead of you.

[pawelbaran]

@BHoMBot check core
@BHoMBot check serialisation

[bhombot-ci]

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

• core
• serialisation

There are 27 requests in the queue ahead of you.

[bhombot-ci]

@pawelbaran 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 @pawelbaran on Revit_Toolkit

[bhombot-ci]

@pawelbaran 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 @pawelbaran on Revit_Toolkit

[FraserGreenroyd]

@BHoMBot check required

[bhombot-ci]

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

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

There are 2 requests in the queue ahead of you.

[michal-pekacki]

LGTM

[pawelbaran]

@BHoMBot check copyright-compliance
@BHoMBot check dataset-compliance

[pawelbaran]

@BHoMBot check copyright-compliance
@BHoMBot check dataset-compliance

[bhombot-ci]

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

• copyright-compliance
• dataset-compliance

[FraserGreenroyd]

@BHoMBot check ready-to-merge

[bhombot-ci]

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

• ready-to-merge

There are 2 requests in the queue ahead of you.