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

Revit faces convert to bhom isurfaces #676

Conversation

tiagogrossi
Copy link
Contributor

@tiagogrossi tiagogrossi commented Apr 24, 2020

Issues addressed by this PR

Closes #454

Created convert methods to extract Revit Face to BHoM ISurface. The interface for this to happen is the same as the convert ICurve in RevitPullConfig, with an additional entry called pullSurfaces (maintained pullEdges).

Only PlanarSurface is implemented in this PR, other surface types to follow.

Test files

Both GH and Revit files to test are here

@tiagogrossi tiagogrossi added the status:WIP PR in progress and still in draft, not ready for formal review label Apr 24, 2020
@tiagogrossi tiagogrossi requested a review from pawelbaran April 24, 2020 09:57
@tiagogrossi tiagogrossi self-assigned this Apr 24, 2020
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

In general looks good, works nicely on pull, a few changes requested in the code comments.

Adapter_Revit_UI/CRUD/Read.cs Outdated Show resolved Hide resolved
Engine_Revit_UI/Convert/Geometry/FromRevit/Surface.cs Outdated Show resolved Hide resolved
Engine_Revit_UI/Convert/Geometry/FromRevit/Surface.cs Outdated Show resolved Hide resolved
Engine_Revit_UI/Convert/Geometry/FromRevit/Surface.cs Outdated Show resolved Hide resolved
Revit_oM/Config/RevitPullConfig.cs Show resolved Hide resolved
@pawelbaran
Copy link
Member

By the way, @tiagogrossi could you please update the PR title following the PR instructions? Thanks!

@tiagogrossi tiagogrossi changed the title Revit toolkit issue454 revit faces convert to bhom isurfaces Revit faces convert to bhom isurfaces Apr 24, 2020
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

Works fine functionality wise - I would say it is ready to merge once the comments below are resolved.

@pawelbaran
Copy link
Member

@FraserGreenroyd I believe it would be nice to have your review here because:
a) we need a green light in general 😆
b) this functionality might be really useful for BEnv - may be worth spreading the word

@FraserGreenroyd
Copy link
Contributor

@FraserGreenroyd I believe it would be nice to have your review here because:

PR is still in WIP at the moment so I'm focusing on other tasks - if the PR is ready for review then I'm happy to give it a glance over but a proper test would have to wait till the weekend I'm afraid 😞

@tiagogrossi tiagogrossi added type:feature New capability or enhancement and removed status:WIP PR in progress and still in draft, not ready for formal review labels Apr 24, 2020
@tiagogrossi
Copy link
Contributor Author

@FraserGreenroyd I just finished addressing Pawel's comments and changed this to feature. There is no rush to get this merged.

Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

LGTM

@pawelbaran
Copy link
Member

/azp run Revit_Toolkit.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FraserGreenroyd
Copy link
Contributor

/azp run Revit_Toolkit.CheckCompliance

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FraserGreenroyd FraserGreenroyd merged commit d9992f7 into master Apr 27, 2020
@FraserGreenroyd FraserGreenroyd deleted the Revit_Toolkit-Issue454-Revit-faces-convert-to-BHoM-iSurfaces branch April 27, 2020 14:27
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.

Revit_Toolkit: Implement converts from Revit surfaces to BHoM
3 participants