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

Dfx splines #370

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Dfx splines #370

wants to merge 13 commits into from

Conversation

jcoffland
Copy link
Member

This is a branch of PR #367. It fixes some issues with #367. These PRs aim to resolve issue #366.

Comment on lines +198 to +213
var npts = spl.ctrlPts.length

if (spl.isClosed) {
// TODO Is it correct to rewrite the knots values?
npts += degree + 1

var delta = 1.0 / (npts + degree + 1)
var knot = 0
var knots = []

for (let i = 0; i < npts + degree; i++) {
knots.push(knot)
knot += delta
}

for (var i = 1; i <= 100; i++) {
var u = quad_bezier(p, 0.01 * i);
l += distance2D(v, u);
v = u;
knots.push(1.0)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm confused by this code. If the number of control points is n + 1 and the degree is p then by the end of this code the number of knots is n + 2 * p + 2 but bspline-curve-closed says the number of knots should be m + 1 where m = n + p + 1. In other words, the number of knots should be n + p so it's p + 2 too many.

Note, I did modify this code a bit but I think the calculation is the same. Also, if you correct this code it breaks the spline evaluation, so something else must be wrong too.

Choose a reason for hiding this comment

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

degree = p; @ 197
npts = n+1; @ 198
npts = n+1+p+1; @ 202

npts + degree = n+1+p+1+p; @ 208
m+1 = n+1+p+1+p
m = n+1+p+p.

So yes, it looks like there are p extra knots...
I wonder if line 202 should be 'npts += 1' instead. Is that the correction you tried?

Choose a reason for hiding this comment

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

I checked the code that I committed and it looks like I had:

function nurbs_build_knots(spl) {
  var degree = spl.degree;
  var npts = spl.ctrlPts.length;
  var knots = [];

  if(spl.isClosed) {
      /* TODO: is it right to rewrite the knots values? */
      npts = npts+degree*2;
      var delta = 1.0/(npts+degree+1);
      var knot = 0.0;
      for(var i=0; i<npts+degree; i++) {
        knots.push(knot);
        knot+=delta;
      }
      knots.push(1.0);
  } else {

So I had a loop of npts+degree=n+1+p2+p, so m+1=n+p+1+p2 which had p*2 too many knots, according to that page. I remember that I needed that many knots to allow nurbs_interpolate to work more or less in the same way for open and closed nurbs.

I just tried to remove that npts = npts+degree*2 and modify the nurbs_interpolate accordingly to take into consideration the fewer knots, but something is not right with the min/max calculations for the t = min + (max-min) * t; range of closed nurbs: it looks like var max = knots[knots.length-degree+1] draws too much on one end, but var max = knots[knots.length-degree] too much on the other end.

My test:

diff --git a/tpl_lib/dxf/dxf.tpl b/tpl_lib/dxf/dxf.tpl
index e8480d1..5104ddc 100644
--- a/tpl_lib/dxf/dxf.tpl
+++ b/tpl_lib/dxf/dxf.tpl
@@ -155,7 +155,7 @@ function get_segment_index(knots, t, degree, is_closed) {
   /* TODO: can this be unified? */
   if(is_closed) {
     for(var segment = 0; segment<knots.length; segment++) {
-      if(knots[segment]>t) return segment-1;
+      if(knots[segment]>=t) return segment-1;
     }
   } else {
     for(var segment = degree; segment<knots.length; segment++) {
@@ -208,7 +208,6 @@ function nurbs_build_knots(spl) {
   var knots = [];
   if(spl.isClosed) {
       /* TODO: is it right to rewrite the knots values? */
-      npts = npts+degree*2;
       var delta = 1.0/(npts+degree+1);
       var knot = 0.0;
       for(var i=0; i<npts+degree; i++) {
@@ -249,8 +248,8 @@ function nurbs_interpolate(t, spl, vertices, knots) {
   }
 
   if(spl.isClosed) {
-    var min = knots[degree+1];
-    var max = knots[knots.length-degree-1];
+    var min = knots[degree-1];
+    var max = knots[knots.length-degree];
     t = min + (max-min) * t;
   } else {
     var min = knots[degree];
@@ -287,10 +286,11 @@ function nurbs_interpolate(t, spl, vertices, knots) {
     }
   }
 
+       segment=(segment+npts)%npts;
   return {
-    x: verts[segment%npts][0] / verts[segment%npts][3],
-    y: verts[segment%npts][1] / verts[segment%npts][3],
-    z: verts[segment%npts][2] / verts[segment%npts][3]
+    x: verts[segment][0] / verts[segment][3],
+    y: verts[segment][1] / verts[segment][3],
+    z: verts[segment][2] / verts[segment][3]
   } 
 }
 

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I remember now I did reduce the number of knots by p. I'm still bothered by not knowing why it works correctly with seemingly the wrong number of knots.

for (let segment = spl.degree; segment < knots.length - 1; segment++)
if (knots[segment] <= t && t <= knots[segment + 1])
return segment

Copy link
Member Author

Choose a reason for hiding this comment

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

Note, I removed some dead code here. if (!knot) was always false.

var npts = spl.ctrlPts.length

if (spl.isClosed) {
// TODO Is it correct to rewrite the knots values?
Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. Why cant we use the knot vector supplied in the DXF file. I suppose some DXF files may be missing the knot vector. This code assumes a uniform knot vector which may not agree with the DXF.

Choose a reason for hiding this comment

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

The bspline-curve-closed page has two methods to close the nurb. I implemented 'Wrapping Control Points', but it also has a 'Wrapping Knots' method that mentions 'These knots are not necessarily uniform, an advantage over the method discussed above', so maybe that's more correct when the dxf specifies control points?
If you want, you can give it a try and see if works better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to figure this out. The DXF files I've seen contain knot vectors in this format:

0,0,0,d,2*d,3*d,. . .,(n-1)*d,1,1,1

Where d is the uniform knot delta. Obviously in a closed, periodic spline you don't need the leading 0's and trailing 1's but how to you convert the knot vector? Part of the problem is the number of knots to control points in a DXF file does not quite make sense to me. I suppose we could just forego non-uniform splines and always generate a uniform knot vector. Perhaps non-uniform splines don't often occur in DXF files. We could use some more DXF file spline examples. Really we need a test suite of DXF inputs.

Choose a reason for hiding this comment

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

Same here... I diffed a few open and closed splines from LibreCAD and all the knots/control_points were the same; the only diff was in the isClosed flag.
Maybe we can try to take a look at LibreCAD's rendering code since it's open source?
But yeah, I'm not sure how often non-uniform splines are encountered in CADs.
An option could be to assert that the knots are in that uniform format you described above and pop-up a warning when someone tries to load non-uniform splines, but asserting on floating point values is always a bit risky...

Copy link
Member Author

Choose a reason for hiding this comment

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

I did already look at LibreCAD's spline code. It uses a library called opennurbs. In the process of calling in to opennurbs, LibreCAD also appears to throw out the knot vector supplied by the DXF file.

@luzpaz
Copy link
Contributor

luzpaz commented Jun 11, 2023

PR name should be DXF splinesnot Dfx splines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants