Codecov Report

Merging #636 (8d2c0b9) into master (d1ebfba) will increase coverage by 0.07%.
The diff coverage is 97.70%.

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
+ Coverage   93.94%   94.02%   +0.07%     
==========================================
  Files          31       31              
  Lines        6543     6627      +84     
  Branches      713      724      +11     
==========================================
+ Hits         6147     6231      +84     
  Misses        258      258              
  Partials      138      138              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1ebfba...8d2c0b9. Read the comment docs.

What is exactly the use case for parameters? Does not scaling change the geometry or result in an error in the general case?

Regarding black, we use the defaults of 19.10b0.

Github was being annoying when I tried to comment on the docstring for Edge.makeSpline, since the comment touched on lines @pavpen hadn't written, so I added a commit instead.

Aside from my comments this is all looking pretty good to me. Nice first contribution @pavpen!

@pavpen any thoughts regarding my questions above?

What is exactly the use case for parameters? Does not scaling change the geometry or result in an error in the general case?

I mostly set the value of the curve's parameter at different interpolation points, so that I can easily select a point between two interpolation points I'm interested in.

I don't quite understand your second question. Scaling the parameter has no effect on the shape of the curve (provided that the tangents are scaled correspondingly, if you're specifying tangents). It's not an error to choose any parameter values at the various interpolation points, as long as the parameter is monotonically increasing (theoretically, it could work if it's decreasing, too, but OCC seems to throw an error in this case) as you go from interpolation points at the beginning of the curve towards points at the end of the curve.

Here's an example of approximating an Archimedean spiral with a B-spline. (You can see which curve looks best.)

# -*- coding: utf-8 -*-

from cadquery.cq import Workplane
import cadquery as cq

import math

# Helper functions for defining an Archimedean spiral [Δr(θ) ∝ Δθ]:

# Spiral parameters:
center = cq.Vector(0, 0)
r0 = 1
angle0 = 0
dr_dthetaRadians = 10 / (2 * math.pi)


def point_at_angle(angular_distance):
    r = r0 + dr_dthetaRadians * angular_distance
    angle = angle0 + angular_distance

    return center + cq.Vector(r * math.cos(angle), r * math.sin(angle))


def tangent_at_angle(angular_distance):
    r = r0 + dr_dthetaRadians * angular_distance
    angle = angle0 + angular_distance

    return cq.Vector(
        (-r * math.sin(angle) + dr_dthetaRadians * math.cos(angle)),
        (r * math.cos(angle) + dr_dthetaRadians * math.sin(angle)),
    )


# Calculate interpolation points for the spiral:
angles = [i * math.pi / 2 for i in range(4 * 3)]
points = [point_at_angle(a) for a in angles]
tangents = [tangent_at_angle(a) for a in angles]
parameters_point_index = range(len(points))
parameters_angle = angles

# Connect the interpolation points with B-splines, using different values for
# the B-spline parameter at the interpolation points:
spiral_with_default_parameters = cq.Workplane("XY").spline(
    points, tangents=tangents, scale=True, includeCurrent=False,
)
spiral_with_point_index_parameters = cq.Workplane("XY").spline(
    points,
    tangents=tangents,
    parameters=parameters_point_index,
    scale=False,
    includeCurrent=False,
)
spiral_with_scaled10_point_index_parameters = cq.Workplane("XY").spline(
    points,
    tangents=tangents,
    parameters=[p * 10 for p in range(len(points))],
    scale=True,
    includeCurrent=False,
)
spiral_with_scaled100_point_index_parameters = cq.Workplane("XY").spline(
    points,
    tangents=tangents,
    parameters=[p * 100 for p in range(len(points))],
    scale=True,
    includeCurrent=False,
)
spiral_with_angle_parameters = cq.Workplane("XY").spline(
    points,
    tangents=tangents,
    parameters=parameters_angle,
    scale=False,
    includeCurrent=False,
)
spirals_with_scaled_parameters = (
    Workplane("XY")
    .add(spiral_with_scaled10_point_index_parameters)
    .add(spiral_with_scaled100_point_index_parameters)
)


# Save the B-splines to files:
if __name__ == "__main__":
    workplanes_dict = dict(
        [(k, v) for k, v in locals().items() if isinstance(v, cq.Workplane)]
    )
    for var_name, workplane in workplanes_dict.items():
        cq.exporters.export(workplane, f"$Demo_{__file__}-{var_name}.svg")

Image above: spiral_with_default_parameters, i.e. no parameters specified when interpolating

Image above: spiral_with_point_index_parameters, i.e. parameter (at an interpolation point) = interpolation point index

Image above: spiral_with_scaled10_point_index_parameters, i.e. parameter (at an interpolation point) = 10 × interpolation point index

Image above: spiral_with_scaled100_point_index_parameters, i.e. parameter (at an interpolation point) = 100 × interpolation point index

Image above: spiral_with_angle_parameters, i.e. parameter = spiral angle

Image above: spirals_with_scaled_parameters, i.e. two spirals overlayed: parameter (at an interpolation point) = 10 × interpolation point index, and parameter = 100 × interpolation point index.

Don't be confused, if you don't see two spirals 😊.

Regarding black, we use the defaults of 19.10b0.

I saw that when the Jenkins test failed. I think environment.yml should be updated to pin the version of black. It's also worth mentioning in the documentation (i.e., README.md, unless there are other paces, too). I could write another issue for that when I get a chance.

@pavpen thanks, second question was about the tangents. You answered it in the review comments.

If we check the length of tangents and raise a ValueError, should we be doing the same for parameters? I don't think it's valid to have a different number of parameters to interpolation points.

I'm a bit worried I have not handled periodic=True properly in that latest commit. I can't get to my computer at the moment, I'll check it in an hour or so.

Damn, my code was passing black but I was using version 20.8b1 to check. Needs 8d2c0b9 to pass with 19.10.

If we check the length of tangents and raise a ValueError, should we be doing the same for parameters? I don't think it's valid to have a different number of parameters to interpolation points.

👍 I see you added a commit for this.

I think it's a good idea. The OCC library should throw an exception, if we don't check, but I think Python exceptions are much easier to understand. The ones coming from C++ often don't explain the reason for the exception.

Thankyou very much @pavpen, this is a great contribution. Thanks also for the detailed comments and examples, they made reviewing much easier.

+1, big thanks for the contributions and patience @pavpen .

Thanks for all the help.

I have a few more minor suggestions that I can write issues for.

Also, I kind of hacked the SVG exporter so I can generate the SVGs that I used for the examples above. I changed the styling, so the graphic works both on bright and on dark backgrounds (like GitHub's dark theme). I was thinking it may be useful to add that as a feature, as well. Although it may require a bit of discussion.

Sounds good, let's discuss in a new issue @pavpen .