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

Calls to new PlanarSurface(ICurve, null) replaced with BH.Engine.Geometry.Create.PlanarSurface #1346

Merged

Conversation

pawelbaran
Copy link
Member

Issues addressed by this PR

Closes #1345

Test files

As in #1345, also standard procedure for pull of panels needs to be re-tested.

Changelog

Additional comments

@pawelbaran pawelbaran added the type:bug Error or unexpected behaviour label Apr 5, 2023
@pawelbaran pawelbaran requested a review from vietle-bh April 5, 2023 14:59
@pawelbaran pawelbaran self-assigned this Apr 5, 2023
@pawelbaran
Copy link
Member Author

@BHoMBot check core
@BHoMBot check compliance

5 similar comments
@pawelbaran
Copy link
Member Author

@BHoMBot check core
@BHoMBot check compliance

@pawelbaran
Copy link
Member Author

@BHoMBot check core
@BHoMBot check compliance

@pawelbaran
Copy link
Member Author

@BHoMBot check core
@BHoMBot check compliance

@pawelbaran
Copy link
Member Author

@BHoMBot check core
@BHoMBot check compliance

@pawelbaran
Copy link
Member Author

@BHoMBot check core
@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 5, 2023

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

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

@vietle-bh
Copy link
Contributor

vietle-bh commented Apr 6, 2023

In the issue, the link for test files only links to the issue itself.

@pawelbaran
Copy link
Member Author

I fixed the link @vietle-bh, test files are here.

Copy link
Contributor

@vietle-bh vietle-bh left a comment

Choose a reason for hiding this comment

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

It worked on the test file you provide but raised some errors on the test scripts below:

Test scripts

image

image

@pawelbaran
Copy link
Member Author

Oke, it sounds like we again enjoy the differences between Revit and BHoM tolerances, the latter being more strict, therefore causing errors - thank you for spotting @vietle-bh 👍 I have restored the constructors instead of Create methods where applicable (e.g. on planar face converts - no need to re-do all planarity checks etc.), the only tweak now being passing new List<ICurve> instead of null, which should eliminate the original error reported in #1345.

@pawelbaran pawelbaran requested a review from vietle-bh April 13, 2023 17:38
@pawelbaran
Copy link
Member Author

@BHoMBot check compliance
@BHoMBot check core

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 13, 2023

@pawelbaran 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
  • check core

Copy link
Contributor

@vietle-bh vietle-bh left a comment

Choose a reason for hiding this comment

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

All tests have passed

@pawelbaran
Copy link
Member Author

@BHoMBot check null-handling
@BHoMBot check serialisation
@BHoMBot check versioning
@BHoMBot check installer

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 14, 2023

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

  • check null-handling
  • check serialisation
  • check versioning
  • check installer

There are 19 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 14, 2023

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

@pawelbaran
Copy link
Member Author

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Apr 14, 2023

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

  • check ready-to-merge

@pawelbaran pawelbaran merged commit 01299c0 into develop Apr 14, 2023
@pawelbaran pawelbaran deleted the Revit_Toolkit-#1345-PlanarSurfaceConstructorIssues branch April 14, 2023 15:35
@bhombot-ci bhombot-ci bot mentioned this pull request Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code calling new PlanarSurface(ICurve, null) to create a surface causes issues downstream
2 participants