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

Line3 accepts arbitrary moment and direction vector arguments #61

Closed
tahsinkose opened this issue Dec 11, 2022 · 8 comments
Closed

Line3 accepts arbitrary moment and direction vector arguments #61

tahsinkose opened this issue Dec 11, 2022 · 8 comments
Labels
enhancement New feature or request

Comments

@tahsinkose
Copy link

Line3 currently accepts an arbitrary combination of moment and direction vectors, which could directly result in an invalid Plücker coordinates representation. Nevertheless, a valid Plücker coordinate representation is guaranteed in the docs, which is wrong. The following snippet illustrates this neatly:

v = np.array([2, 2, 3])
w = np.array([-3, 2, 1])
tw = Twist3( v, w)
recv_line = tw.line()
assert abs(np.dot(recv_line.v, recv_line.w)) < 1e-4

I believe the root cause of this issue is the mismatch between docs and implementation. The associated docs suggest that the input is a valid Plücker representation, whereas the implementation does not check for the validity of the input and assumes it is valid. Therefore, the test above fails.

@petercorke
Copy link
Collaborator

Nice catch. What should the action be? For the SO3 class the constructor checks validity if check=True (default) and throws an error if the matrix is invalid. Could do a similar thing here? If you know you have a valid direction/moment then set check=False to skip the check and go faster. Thoughts?

@petercorke petercorke added the enhancement New feature or request label Jan 4, 2023
@tahsinkose
Copy link
Author

@petercorke please check https://github.com/petercorke/spatialmath-python/pull/62#issue-1490372754. It turns out that Line3 is a general abstraction, which could be extended by Plücker-Screw-Twist coordinates, each of which has different validity conditions.

I think the documentation for Line3 should reflect this reality, i.e. the fact that it is a general abstraction of 3D lines. Please also have a look at https://github.com/petercorke/spatialmath-python/pull/62. I can adjust the documentation of Line3 there and we can merge all of them together.

@petercorke
Copy link
Collaborator

I worry about the proliferation of classes. There is already a Twist3 class which is a motion, and Line3 which is a 3D line. What extra does Screw provide? I can see value in:

  • creating a pure Plucker coordinate class that Line3 inherits from and adds all the line-specific stuff
  • the Plucker class could have all the checking
  • being able to create a Twist3 from a Line3, pitch and θ

We can already convert a Twist3 (motion) to a Line3 (it's line of action).

The reason Plucker has the deprecation warning is that was the name of the Line3 class in the MATLAB version of the toolbox.

@tahsinkose
Copy link
Author

tahsinkose commented Jan 5, 2023

What extra does Screw provide?

I created Screw to explicate the difference between Screw vs. Plucker vs. Twist. Its existence better stresses the meaning and differences of pitch and θ IMHO. At least, that was my experience when learning the subject. Besides, they are different geometric objects, hence I believe it is better to keep them separate to avoid possible ambiguities.

creating a pure Plucker coordinate class that Line3 inherits from and adds all the line-specific stuff

I think their dependency should be in a reverse way, i.e. Plucker inherits from Line3. The rationale is that Line3 does not currently do a strict unit-vector check on the inputs. Instead, it only checks whether the inputs are 3-vectors and I think that is sufficient for a base class. Plucker, Screw, and Twist are all specializations of Line3 (i.e. a pair of 3-vectors) and implement their own validity checks in their constructors.

the Plucker class could have all the checking

Not sure whether you have checked https://github.com/petercorke/spatialmath-python/pull/62, but I already implemented this here 🙂

being able to create a Twist3 from a Line3, pitch and θ

Also here.

@petercorke
Copy link
Collaborator

A 3D line and a 3D helicoidal motion are distinct things, currently Line3 and Twist3. I don't know what the third thing is. The lack of parameter checking in Line3 was an oversight not a deliberate design decision, and as previously stated it is really easily fixed. What is the use case for parameters that don't meet a "strict unit-vector check"?

The Line3 class has lots of baggage for plotting and plane/volume intersections which a pure Plucker coordinate class wouldn't need, hence why I suggested that inheritance order.

@tahsinkose
Copy link
Author

tahsinkose commented Jan 6, 2023

My concern is the back-compatibility for avoiding adding that check to Line3. Introducing a strict unit-vector check to Line3 might break existing usages. This is not particularly a production framework, although I guess there are many active users whose code already utilizes Line3.

The Line3 class has lots of baggage for plotting and plane/volume intersections which a pure Plucker coordinate class wouldn't need, hence why I suggested that inheritance order.

For this exact reason, I thought of leveraging all that goodies in Plucker and Screw classes through the opposite inheritance order. If someone wants to visualize or do geometric computations with those classes, their base class Line3 would trivially provide that. The inheritance order you suggested would require a Plucker to be converted to a Line3 for those operations.

I don't know what the third thing is

AFAIU a Twist3 is a normalized Screw coordinates representing the motion (kinematics?) of a rigid-body object, thus we can consider the former as an application/specialization of the latter. Similarly, a Wrench is another application of Screw representing the forces/torques (dynamics?). In fact, SpatialVector class already implements the closest representation of Screw.

@petercorke
Copy link
Collaborator

Thanks for your input on this. I'm going to close this out with a minimal change now on master branch which checks the 6D coordinate for validity (but check can be turned off).

@tahsinkose
Copy link
Author

@petercorke could you please provide a link to commits/PR that has this new validity check? So that I can update my fork accordingly.

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

No branches or pull requests

2 participants