search

BHoM / Rhinoceros_Toolkit

Set of functionalities for communication with Rhinoceros
GNU Lesser General Public License v3.0
6 stars 2 forks source link

Create Rhinoceros adapter #205

Closed rolyhudson closed 1 year ago

rolyhudson commented 3 years ago

Issues addressed by this PR

Rudimentary adapter set up for very basic pull and push.

Closes #199

Test files

Simple pull and push

Changelog

Additional comments

rolyhudson commented 3 years ago

Refactored this now after testing approaches for very basic workflows. Anticipating further development to handle requests and more complex CRUD operations. This PR aims to capture the read workflow indicated in #199 plus a simple create file workflow.

For the Pull Action this means simply read everything from the specified file. Objects are stored as the BH.oM.Adapters.Rhinoceros.RhinoObject capturing geometry, object colour and layer.

For the Push Action the initial workflow is seen as simply writing to a new file or overwriting if an existing file is specified.
To do so Push is overridden. Why?

  1. This provides the mechanism to create a single 3dm file object where objects added.
  2. To enforce that this adapter (for now) is PushType.CreateOnly and expose a warning for users that this is the case.

Would be good to get @IsakNaslundBh + @alelom's views on this.

rolyhudson commented 3 years ago

/azp run Rhinoceros_Toolkit.CheckInstaller

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
FraserGreenroyd commented 3 years ago

@rolyhudson and @alelom (and @IsakNaslundBh to a lesser extent). Given this PR is described as a rudimentary adapter set up for very basic pull and push and this wasn't available in the 4.0 beta, can I check we are happy with the use cases/risks vs reward of merging this in now for inclusion in the 4.1 beta, vs waiting for a merge in the first sprint of 4.2, getting some alpha testing under its belt for inclusion in the 4.2 beta.

Not wanting to hold things up unnecessarily, just raising the question for discussion to ensure we're happy that this work will be deployed in installers globally in 2 weeks without much opportunity for alpha users to feedback :smile:

IsakNaslundBh commented 3 years ago

Given this PR is described as a rudimentary adapter set up for very basic pull and push and this wasn't available in the 4.0 beta, can I check we are happy with the use cases/risks vs reward of merging this in now for inclusion in the 4.1 beta, vs waiting for a merge in the first sprint of 4.2, getting some alpha testing under its belt for inclusion in the 4.2 beta.

From my point of view, I think it is fine to wait, unless @rolyhudson and @alelom disagrees due to needs for some workflows.

rolyhudson commented 3 years ago

I am happy to wait and test fully in 4.2 alpha's. For me it was an opportunity to look deeper into adapters so no need for haste in merging.

FraserGreenroyd commented 3 years ago

@BHoMBot check ready-to-merge

bhombot-ci[bot] commented 3 years ago
@FraserGreenroyd to confirm, the task for checking if this Pull Request is ready to merge is now queued.
FraserGreenroyd commented 1 year ago

Following discussion in today's PR closure day with @IsakNaslundBh and @al-fisher we're closing this PR as it has a large amount of conflicting files with other work done to this toolkit, but the branch will remain open to support @michaelhoehn or others in resolving the issue of a Rhino Adapter in the future 😄