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

Geometry_Engine: Remove Degrees() method for NurbsSurface #1298

Conversation

pawelbaran
Copy link
Member

@pawelbaran pawelbaran commented Nov 4, 2019

!!! PLEASE DO NOT MERGE !!!

NOTE: Depends on

#603

Issues addressed by this PR

Closes #1297

Test files

Test file is located on SharePoint. To run it, please switch to the branches related to the following PRs:

The error in the test file is caused by the bug in Cone convert logged here.

Changelog

  • BH.Engine.Geometry.Query.Degrees(this NurbsSurface surf) has been removed.

Additional comments

Please see #603.
The PR should not conflict with #1283 as there is no overlap.

@pawelbaran pawelbaran added type:feature New capability or enhancement type:question Ask for further details or start conversation status:WIP PR in progress and still in draft, not ready for formal review labels Nov 4, 2019
@pawelbaran pawelbaran self-assigned this Nov 4, 2019
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.

Hi @pawelbaran - fully recognise this is a WIP bit of work in conjunction with BHoM/BHoM#603 however, the branch names should be the same in order to allow the CI checks to pass - currently the engine is failing because it cannot find the suitable PR on the core oM.

I recommend branching off your current branch to the correct branch name (matching the oM one) and then remaking the PR - but that can of course be done at the time you're ready for the work to be merged - this is just an advanced heads up 😄

Copy link
Member

@epignatelli epignatelli left a comment

Choose a reason for hiding this comment

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

Maybe it is a trivial question, but better ask for forgiveness 😄 have you made sure that Degrees is not used anywhere else? Other toolkit especially? As I commented in BHoM/BHoM#603 I think it would be safer to create a new TrimmedSurface object, rather than modifying the NurbsSurface one.

This would avoid us scrabbling through the code to check if we are breaking something, and we do not break past scripts. The rationale is that before this modification, this functionality was not there anyway, so no script could have relied on it.

@pawelbaran
Copy link
Member Author

@epignatelli being honest I must say that atm I have not checked all dependencies as the PRs are still WIP. Nevertheless, as explained in #1297, I believe that we need to remove Degrees for the simple reason that it will never work correctly for certain cases.

Concerning TrimmedSurface vs extending NurbsSurface, as in #603, I prefer the latter, but would rather wait for others to speak up. Whatever we decide, I am happy to do the effort and make sure that the change will not break anything - no worries about that.

@pawelbaran
Copy link
Member Author

Closing this one and reopening in #1319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:WIP PR in progress and still in draft, not ready for formal review type:feature New capability or enhancement type:question Ask for further details or start conversation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geometry_Engine: Remove Degrees() method for NurbsSurface
3 participants