Add Support for Specifying Arbitrary Tangents, etc. in spline() #636
Conversation
Codecov Report
@@ 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.
|
* Added support for specifying the tangents at arbitrary interpolation points when interpolating a B-spline curve with `Workplane.spline()`. * Added support for specifying whether the tangents should be automatically scaled. (I.e., only use the tangent vector directions, rather than their magnitudes.) * Added support for specifying the value of the curve function parameter at the interpolation points. * Added support for specifying the interpolator's tolerance value. Q: _There are a number of whitespace, and other formatting changes introduced by `black`. Is there a specific list of parameters that you use when running code formatting?_
pavpen
force-pushed
the
spline-tangents-parameters-tolerance
branch
from
a88443f
to
1caae59
Compare
|
What is exactly the use case for Regarding black, we use the defaults of |
* Reformatted the code with `black` 19.10b. This is the version that's used by the Travis continuous integration check. The default version of `black` that comes with Conda is 20.8b1, which produces different formatting. <environment.yml> should be updated to require the specific version of `black` used to check code before it's merged.
* Updated `Edge.makeSpline()` to use different temporary variables in the several for loops in the function body. Python for [loop target variables leak](https://eli.thegreenplace.net/2015/the-scope-of-index-variables-in-pythons-for-loops/) to the function (or enclosing module) scope, and MyPy doesn't accept [reusing the same variable name with a different type](python/mypy#1174) (except in specific cases, and when `--allow-redefinition` is used).
|
@pavpen any thoughts regarding my questions above? |
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_angle_parameters, i.e. parameter = spiral angle
Don't be confused, if you don't see two spirals 😊.
I saw that when the Jenkins test failed. I think |
* Moved the tests cases, so they're next to each other. * Added a comment about the relationship between the two test cases.
…s` Argument * Changed the type hint to `Sequence` instead of `List`, since other list-like objects are acceptable, as wel.
* Updated `makeSpline` to construct an explicit exception, rather than use an `assert` to check input parameters.
* Updated DocString indentation. Hopefully, that fixes the Sphinx-generated documentation. I can't currently build locally.
|
@pavpen thanks, second question was about the tangents. You answered it in the review comments. |
* Fixed the `testSplineTangentMagnitudeBelowToleranceThrows` unit test. It was failing since `spline()`'s parameter `tolerance` seems to have been renamed to `tol`.
|
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 |
|
Damn, my code was passing black but I was using version 20.8b1 to check. Needs 8d2c0b9 to pass with 19.10. |
👍 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 . |
Workplane.spline().Q: There are a number of whitespace, and other formatting changes introduced by
black. Is there a specific list of parameters that you use when running code formatting?