JdeRobot / RoboticsAcademy

Learn Robotics with JdeRobot
https://jderobot.github.io/RoboticsAcademy
GNU General Public License v3.0
317 stars 223 forks source link

[gh-pages] The laser "Types Conversion" documentation in the Vacuum Cleaner exercise could be improved #2731

Closed franmore-urjc closed 1 month ago

franmore-urjc commented 1 month ago

The problem:

In the documentation for this exercise (and probably others, I didn't check them all), a couple of functions are proposed: parse_laser_data(laser_data) and laser_vector(laser). However, there is zero documentation in that section about what they do. Also, the API says that the laser data is in millimeters, but it seems to be in meters (unless the house is ant-size). These are the snippets in question:

laser_data = HAL.getLaserData()

def parse_laser_data(laser_data):
    laser = []
    for i in range(180):
        dist = laser_data.values[i]
        angle = math.radians(i)
        laser += [(dist, angle)]
    return laser

and

def laser_vector(laser):
    laser_vectorized = []
    for d,a in laser:
        # (4.2.1) laser into GUI reference system
        x = d * math.cos(a) * -1
        y = d * math.sin(a) * -1
        v = (x,y)
        laser_vectorized += [v]

    laser_mean = np.mean(laser_vectorized, axis=0)
    return laser_mean

Moreover, the code snippets make no sense, as in the first the conversion is not adjusting the angle offset (to center the angle at the front of the robot), and the second function is extracting the mean of all the laser values, instead of returning a list of (x,y) coordinates (assuming that is the purpose of the function, which is not clear because there is no text in that section and there are no comments in the function), so they are not usable as they are.

Possible solutions

It would be nice to provide a "parse" function that converts the raw LaserData object that is received by HAL into a list with more useful information, like a list of (distance, angle) tuples or a list of (x, y) values. I propose something like this, and then let the user choose or tweak the function to their needs, but hoping that he/she understands the format of the input data and the conversions that this function is doing:

Option 1:

def parse_laser_data(laser_data):
    """ Parses the LaserData object and returns a tuple with two lists:
        1. List of  polar coordinates, with (distance, angle) tuples,
           where the angle is zero at the front of the robot and increases to the left.
        2. List of cartesian (x, y) coordinates, following the ref. system noted below.

        Note: The list of laser values MUST NOT BE EMPTY.
    """
    laser_polar = []  # Laser data in polar coordinates (dist, angle)
    laser_xy = []  # Laser data in cartesian coordinates (x, y)
    for i in range(180):
        # i contains the index of the laser ray, which starts at the robot's right
        # The laser has a resolution of 1 ray / degree
        #
        #                (i=90)
        #                 ^
        #                 |x
        #             y   |
        # (i=180)    <----R      (i=0)

        # Extract the distance at index i
        dist = laser_data.values[i]
        # The final angle is centered (zeroed) at the front of the robot.
        angle = math.radians(i - 90)
        laser_polar += [(dist, angle)]
        # Compute x, y coordinates from distance and angle
        x = dist * math.cos(angle)
        y = dist * math.sin(angle)
        laser_xy += [(x, y)]
    return laser_polar, laser_xy

# Usage
laser_data = HAL.getLaserData()
laser_polar, laser_xy = parse_laser_data(laser_data)

This way, the laser's format and the reference system are documented, and it should be easier for the users to convert the laser data to whatever format they need. If you also want to put a small image with the reference system, then even better than my sad comment with the x,y axes.

Option 2:

Another alternative, if you wish to keep the previous parse_laser_data and laser_vector functions separated, is to at least improve them with these comments, and fixing the transformation.

def parse_laser_data(laser_data):
    """ Parses the LaserData object and returns a list of (distance, angle) tuples,
        where the angle is centered at the center of the robot and increases to the left.

        Note: The list of laser values MUST NOT BE EMPTY.
    """
    laser_polar = []  # Laser data in polar coordinates (dist, angle)
    for i in range(180):
        # i contains the index of the laser ray, which starts at the robot's right
        # The laser has a resolution of 1 ray / degree
        #
        #                (i=90)
        #                 ^
        #                 |x
        #             y   |
        # (i=180)    <----R      (i=0)
        #
        # Extract the distance at index i
        dist = laser_data.values[i]
        # The final angle is centered (zeroed) at the front of the robot.
        angle = math.radians(i - 90)
        laser_polar += [(dist, angle)]
    return laser_polar

def laser_vector(laser_polar):
    """ Takes a list of (distance, angle) tuples (laser in polar coordinates),
        and returns a list of (x, y) coordinates in meters following this ref. system:
                 ^
                 |x
              y  |
            <----R

        If the list of laser values is empty, returns an empty list.
    """
    laser_xy = []
    for dist, angle in laser_polar:
        x = dist * math.cos(angle)
        y = dist * math.sin(angle)
        laser_xy += [(x, y)]
    return laser_xy

# Usage
laser_data = HAL.getLaserData()
laser_polar = parse_laser_data(laser_data)
laser_xy = laser_vector(laser_polar)

If you think this is a good idea, feel free to adjust the code and comments as much as you like and update the docs. If, however, this is providing too much info to the users and "solving" part of the exercise, at least please remove the current code snippets, because they are misleading.

javizqh commented 1 month ago

The size has already been changed to meters. We will consider the alternative functions

jmplaza commented 1 month ago

Nice improvement @franmore-urjc !. Definitely old parse_laser_data function was outdated and now makes no sense, the same with laser_vector. Let's go with the improved parse_laser_data :-).

Fixed in #2733