RoboDK / RoboDK-API

Implementation of the RoboDK API in different programming languages. The RoboDK API allows simulating and programming any industrial robot (offline and online)
https://robodk.com/doc/en/RoboDK-API.html
Other
237 stars 117 forks source link

Python API: In robomath.py, multiple issues with transl function #129

Closed sjohnson-FLL closed 1 year ago

sjohnson-FLL commented 1 year ago
  1. Documentation states that tx, ty, and tz are all floats. However, if ty is None, tx gets interpreted as a list: xx = tx[0].
  2. Both ty and tz are listed as optional arguments, but if ty is not passed, tz is ignored. This is probably fine, but it's unclear enough that it should be documented.
  3. Both ty and tz are listed as optional arguments, but if ty is passed but not tz, then zz will be set to None. This doesn't immediately cause an exception when the Mat() is constructed on return. Worse, it causes an exception later, when the code calling the API attempts to use the Mat.

For reference, the transl function is below.

def transl(tx, ty=None, tz=None):
    r"""Returns a translation matrix (mm)

    .. math::

        T(t_x, t_y, t_z) = \begin{bmatrix} 1 & 0 & 0 & t_x \\
        0 & 1 & 0 & t_y \\
        0 & 0 & 1 & t_z \\
        0 & 0 & 0 & 1
        \end{bmatrix}

    :param float tx: translation along the X axis
    :param float ty: translation along the Y axis
    :param float tz: translation along the Z axis

    .. seealso:: :func:`~robodk.robomath.rotx`, :func:`~robodk.robomath.roty`, :func:`~robodk.robomath.rotz`
    """
    if ty is None:
        xx = tx[0]
        yy = tx[1]
        zz = tx[2]
    else:
        xx = tx
        yy = ty
        zz = tz
    return Mat([[1, 0, 0, xx], [0, 1, 0, yy], [0, 0, 1, zz], [0, 0, 0, 1]])

Recommended change: adjust code to handle and update documentation accordingly. Backwards-compatibility is preserved in most cases, though some edge cases like passing tz but not ty will now behave differently (though this was likely not intended to be supported in the first place).

def transl(tx:float|list[float]=0, ty:float=0, tz:float=0):
    r"""Returns a translation matrix (mm) given translations in each dimension. Supports passing inputs as a list through the tx argument, but
    this ignores ty and tz.

    .. math::

        T(t_x, t_y, t_z) = \begin{bmatrix} 1 & 0 & 0 & t_x \\
        0 & 1 & 0 & t_y \\
        0 & 0 & 1 & t_z \\
        0 & 0 & 0 & 1
        \end{bmatrix}

    :param float tx: translation along the X axis
    :param float ty: translation along the Y axis
    :param float tz: translation along the Z axis

    .. seealso:: :func:`~robodk.robomath.rotx`, :func:`~robodk.robomath.roty`, :func:`~robodk.robomath.rotz`
    """
    if type(tx) == list:
        # optionally: assert ty=0 and tz=0, 'TyAndTzUnsupportedWhenPassingTranslationAsListThroughTx'
        xx = tx[0]
        yy = tx[1]
        zz = tx[2]
    else:
        xx = tx
        yy = ty
        zz = tz
    return Mat([[1, 0, 0, xx], [0, 1, 0, yy], [0, 0, 1, zz], [0, 0, 0, 1]])
sambertrdk commented 1 year ago

See https://github.com/RoboDK/RoboDK-API/commit/e436f8ccfb824a3d320a48e57d6119e39069a09c. Thank you!