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

Fix ToRhino for Nurbs #136

Closed
wants to merge 23 commits into from

Conversation

kThorsager
Copy link
Contributor

@kThorsager kThorsager commented Feb 5, 2020

Issues addressed by this PR

Closes #135
Issue: After some testing it shows that the old code worked for Rhino 5, but is broken in Rhino 6...

Rhino 6 stores the control points with the weight factored in, added a fix to pre-factor for the convert.

Test files

https://burohappold.sharepoint.com/:u:/s/BHoM/EZrnbeaXbXtGonmQl3_B_mkB1qBHpJxzSLdiZHQyUsxNpw?e=tsX2tj

Changelog

Additional comments

@kThorsager kThorsager added type:bug Error or unexpected behaviour status:WIP PR in progress and still in draft, not ready for formal review labels Feb 5, 2020
@kThorsager kThorsager self-assigned this Feb 5, 2020
@epignatelli
Copy link
Member

Alright, I think after a chat with @kThorsager and @alelom we got to the bottom of this.
This now works in rhino 6 but not in rhino 5. The master branch shows the opposite behaviour.

The Rhino.Geometry.ControlPoint in RhinoCommon v5 and RhinoCommon v6 do behave differently, inasmuch as they changed the representation of ControlPoints. This is explicit in their documentation as shown below:

Rhino 5
ED4D808D

Rhino 6
ED4D808D

I think this has to be solved differently if we want to support both versions.

I am raising an issue about this, flagging it into other channels to make sure people know about it, and we can discuss about a solution.
I will close this for now.

@epignatelli epignatelli closed this Feb 6, 2020
@epignatelli
Copy link
Member

Reopening so as to highlight the fixes for whoever wants to temporarily jump on this.

@epignatelli epignatelli reopened this Feb 6, 2020
@epignatelli epignatelli added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Feb 6, 2020
@kThorsager
Copy link
Contributor Author

The latest commit will solve the issue of nurbs curves not converting properly in both versions.
This has been done without checking if other converts are working properly.

@epignatelli epignatelli marked this pull request as ready for review March 6, 2020 14:47
@kThorsager
Copy link
Contributor Author

Solved in #148

@kThorsager kThorsager closed this Mar 11, 2020
@epignatelli epignatelli deleted the Rhinoceros_Toolkit-135-ToRhino-for-Nurbs branch May 14, 2020 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge status:WIP PR in progress and still in draft, not ready for formal review type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rhinoceros_Toolkit: ToRhino for Nurbs
2 participants