Refactor ROS parametrization

[nachovizzo]

Expose KISS parameters in the launch file leaving the ROS arguments exposed through the launch configuration

Besides being a more standard and "ROS"y practice, this will allow us in the future for a more flexible configuration of the ROS NODE. The number of parameters on the ROS side of things is likely to grow, and maintaining this in the launch file will become a mess.

This also lets users have a more flexible usage of the ROS node for experimentation, one can now select at launch time with a parameter file to use. In fact, many people are already doing this

**Important **: This PR does not change any piece of the implementation, it's just a cosmetic change on how we manipulate the ROS wiring, nothing else

[tizianoGuadagnino]

I am not super excited about this; it goes a bit against our initial philosophy of separating stuff. I will instead have a slightly larger launchfile that merges the two configs. As we know, the parameters of the base pipeline usually work without intervention, so it makes sense that the use of "tweak" is just the ros part in a specific launch file. Also I think it gives more flexibility for developers working on non-ROS framework (if any). As always we can discuss about it

[nachovizzo]

I am not super excited about this; it goes a bit against our initial philosophy of separating stuff. I will instead have a slightly larger launchfile that merges the two configs. As we know, the parameters of the base pipeline usually work without intervention, so it makes sense that the use of "tweak" is just the ros part in a specific launch file. Also I think it gives more flexibility for developers working on non-ROS framework (if any). As always we can discuss about it

I'm also not super as well, let me see if I can improve the launch.py to merge the configs there

[nachovizzo]

@tizianoGuadagnino please re-review.

Now I merged the YAML file into the launch.py. The only parameters that are exposed to the ROS CLI are:

base_frame = LaunchConfiguration("base_frame", default="")
odom_frame = LaunchConfiguration("odom_frame", default="odom")
publish_odom_tf = LaunchConfiguration("publish_odom_tf", default=True)
pointcloud_topic = LaunchConfiguration("topic")
visualize = LaunchConfiguration("visualize", default="true")
bagfile = LaunchConfiguration("bagfile", default="")

Meaning that any of the above lists can be modified in the launch file but also when invoking it with the cli.

This way KISS remains configured by default, and from outside we can wire different frames for the tf tree. NOTE that this means that this is not possible anymore (but I guess it never made sense anyway)

ros2 launch kiss_icp odometry.launch.py initial_threshold:=0.4

If one needs further customization then one can create a custom launch file to launch the node with the required config:

from launch.substitutions import LaunchConfiguration
from launch_ros.actions import Node


def generate_launch_description():
    return LaunchDescription(
        [
            Node(
                package="kiss_icp",
                executable="kiss_icp_node",
                name="kiss_icp_node",
                output="screen",
                remappings=[("pointcloud_topic", LaunchConfiguration("topic"))],
                parameters=[
                    {
                        "publish_odom_tf": False,
                        "max_range": 20.0,
                    },
                ],
            )
        ]
    )

[tizianoGuadagnino]

Mmmm ok, let me drive it to see how it feels ;)

[tizianoGuadagnino]

I got some weird core dump but I guess I'm doing something wrong. The default Python pipeline kind of dies. I will investigate why, but maybe also @benemer can have a look.

[nachovizzo]

I got some weird core dump but I guess I'm doing something wrong. The default Python pipeline kind of dies. I will investigate why, but maybe also @benemer can have a look.

But I'm not modifying the Python pipeline in this PR 🤔 , maybe is this the new pydantic thing?

EDIT: It is pydantic, is what @benemer also saw, you have an env variable $DATA and pydantic is eating that 🤦 .

I think we might want to change our name to kiss_icp_data or similar:

kiss-icp/python/kiss_icp/config/parser.py

Line 38 in 44d8563

data: DataConfig = DataConfig()

[benemer]

Yes, that's due to the new pydantic. Run

unset DATA 

and it works. @nachovizzo, I think the issue is the data field in the config.yaml that pydantic tries to fill with whatever is in $DATA.

[tizianoGuadagnino]

This is kind of dangerous; it can fire back very easily. We should probably handle this in a separate PR. Me and @benemer will have a look.

[tizianoGuadagnino]

Now back to your thing @nachovizzo

[nachovizzo]

@benemer , @tizianoGuadagnino fix is already on its way !

train of PRs.... 🚅

#286

[tizianoGuadagnino]

Looks good to me :)

[benemer]

To me as well!