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

"Approximate NURBS Curve" node FreeCAD implementation #4881

Merged
merged 22 commits into from
Feb 1, 2023

Conversation

rendetto
Copy link
Contributor

@rendetto rendetto commented Jan 25, 2023

This PR adds the PointsToBSpline OCCT method from FreeCAD for approximating points to a NURBS curve.

Example of Chord Length Parametrization:
FreeCAD_Approx_Parametrization

Example of using Variational Smoothing:
FreeCAD_Approx_Smoothing

EDIT:
I added an option where we can specify the knots explicitly:
FreeCAD_Approx_ExplicitKnots

The cool thing here is that we can utilize the "Generate Knotvector" node and use the metrics from it.

@rendetto
Copy link
Contributor Author

rendetto commented Jan 25, 2023

I need some help with the docs. I'm not sure what I'm doing wrong so that this check is constantly failing.
The problem seems not in the files I'm editing even :/

@Durman
Copy link
Collaborator

Durman commented Jan 26, 2023

image

I guess you have to add blank line after the list item or add 2 spaces in the start of the line after the list item.

@rendetto
Copy link
Contributor Author

rendetto commented Jan 26, 2023

I guess you have to add blank line after the list item or add 2 spaces in the start of the line after the list item.

That's weird because match.rst it's not part of this PR.
Does that mean that failing checks doesn't prevent merging?

@rendetto
Copy link
Contributor Author

rendetto commented Jan 26, 2023

@nortikin Should I try fixing match.rst? I saw you did the last changes.

EDIT: Fixed! :)

@portnov
Copy link
Collaborator

portnov commented Jan 31, 2023

@rendetto it seems you have to rebase this branch to master:

  1. in your master do git pull
  2. in approximate_nurbs_curve_freecad branch do
$ git rebase master
$ git push -f origin approximate_nurbs_curve_freecad

@portnov
Copy link
Collaborator

portnov commented Jan 31, 2023

Ah, merge worked as well :)

@rendetto
Copy link
Contributor Author

rendetto commented Jan 31, 2023

Ah, merge worked as well :)

@portnov is it possible to merge it already? I've done all the changes I was planning.
If you don't find anything alarming please merge it.

Thanks

@portnov
Copy link
Collaborator

portnov commented Jan 31, 2023

Suggestions:

  • Rename "Type" parameter to "Metric".
  • Rename "Uniform" metric to "Uniform (Points)", and "Chord Length" to "Chord Length (Euclidean)"
  • (?) Add support for other metric types. You can calculate knot values with sverchok.utils.geom.Spline.create_knots.

@rendetto
Copy link
Contributor Author

@portnov It's done 2ed47b8 but I'm not very proud :)

all_metrics

Can you take a look at the code changes? I deliberately used the mixed keys:
MANHATTAN, ChordLength, Uniform, CHEBYSHEV, Centripetal, X, Y, Z. It looks wonky but it's the most direct way.
Is it appropriate?

@portnov portnov added the NURBS label Feb 1, 2023
@portnov
Copy link
Collaborator

portnov commented Feb 1, 2023

It's fine IMHO.

@rendetto
Copy link
Contributor Author

rendetto commented Feb 1, 2023

Thanks. I've updated the docs accordingly. It should be ready for merge.

@portnov portnov merged commit 41b60ab into nortikin:master Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants