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

Add method that scrapes data from multiple models #200

Merged

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh commented Feb 9, 2021

NOTE: Depends on

Issues addressed by this PR

Closes #199

Adding a compute method that lets you feed in a list of filepaths and the method returns all geometry objects in those models as well as a concatenated layer name and layer colour.

There are some weirdness with how this is running which makes it trigger multiple times when called from GH.

I think I managed to solve the worst issue by the addition of a static boolean that returns if the method is already running. Not the best solution, but what I could think of on the spot.

This also means the method returns one empty branch and one filled branch in GH. Again, far from ideal, but at least lets you progress.

Would have also liked to add the option of generating a fresh model once it was done, but that command seem to have been added in rhino 6, so not possible for rhino 5 which we still support.

Test files

https://burohappold.sharepoint.com/:f:/s/BHoM/EpskE0cM0mBPpvr5gA3Tu50Bs-4BTimcJ18VxbSqmVXulA?e=oq1QPy

Changelog

Additional comments

As this is not working perfectly, I would be fine with this not being merged, if deemed to unreliable, but raising the PR to get the code up here as it was useful for some current project work (with all its flaws).

Also, if this is in any way clashing with #198 and the representation work, please do not let this get in the way!

@IsakNaslundBh IsakNaslundBh self-assigned this Feb 9, 2021
@IsakNaslundBh IsakNaslundBh added the type:feature New capability or enhancement label Feb 9, 2021
Copy link
Contributor

@rolyhudson rolyhudson left a comment

Choose a reason for hiding this comment

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

Cool may be easier to use the Rhino.FileIO

Rhinoceros_Engine/Compute/CollectAllModelData.cs Outdated Show resolved Hide resolved
Rhinoceros_Engine/Compute/CollectAllModelData.cs Outdated Show resolved Hide resolved
rolyhudson
rolyhudson previously approved these changes Feb 9, 2021
Copy link
Contributor

@rolyhudson rolyhudson left a comment

Choose a reason for hiding this comment

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

Beautiful. 🌷

@adecler
Copy link
Member

adecler commented Feb 10, 2021

/azp run Rhinoceros_Toolkit.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rolyhudson
Copy link
Contributor

Based on conversation earlier we should reopen #67 to create and adapter for Rhino initially to handle the method in this PR.

@IsakNaslundBh IsakNaslundBh added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Mar 12, 2021
@FraserGreenroyd FraserGreenroyd added priority:low Resources should be targeted to other issues first status:parked PR on hold, requiring further discussion or planning labels May 4, 2021
@rolyhudson rolyhudson marked this pull request as draft May 18, 2021 08:57
@rolyhudson
Copy link
Contributor

Converted this to draft to keep it off Fraser's open PR radar.

@IsakNaslundBh IsakNaslundBh changed the base branch from main to develop January 3, 2023 09:21
@IsakNaslundBh IsakNaslundBh dismissed rolyhudson’s stale review January 3, 2023 09:21

The base branch was changed.

@FraserGreenroyd FraserGreenroyd force-pushed the Rhinoceros_Toolkit-#199-CollectDataFromMultipleModels branch from f14bec2 to c8e221a Compare January 9, 2023 11:25
@FraserGreenroyd FraserGreenroyd marked this pull request as ready for review January 9, 2023 11:25
@FraserGreenroyd FraserGreenroyd changed the title Add method that scraps data from multiple models Add method that scrapes data from multiple models Jan 9, 2023
@FraserGreenroyd
Copy link
Contributor

@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 9, 2023

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

  • check versioning

@FraserGreenroyd FraserGreenroyd removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Jan 9, 2023
@FraserGreenroyd FraserGreenroyd removed the status:parked PR on hold, requiring further discussion or planning label Jan 9, 2023
@FraserGreenroyd
Copy link
Contributor

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 9, 2023

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

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

There are 12 requests in the queue ahead of you.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check core
@BHoMBot check null-handling
@BHoMBot check serialisation

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 9, 2023

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

  • check core
  • check null-handling
  • check serialisation

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

I have run a test on 2 files and geometry was pulled in nicely so happy to approve this for deployment via alpha.

Rhinoceros_Engine/Compute/CollectAllModelData.cs Outdated Show resolved Hide resolved
@FraserGreenroyd
Copy link
Contributor

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 9, 2023

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

  • check ready-to-merge

There are 11 requests in the queue ahead of you.

@FraserGreenroyd FraserGreenroyd merged commit d22bf9d into develop Jan 9, 2023
@FraserGreenroyd FraserGreenroyd deleted the Rhinoceros_Toolkit-#199-CollectDataFromMultipleModels branch January 9, 2023 12:00
@bhombot-ci bhombot-ci bot mentioned this pull request Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:low Resources should be targeted to other issues first type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrap multiple models for all of their data
4 participants