ros-perception / vision_msgs

Algorithm-agnostic computer vision message types for ROS.
Apache License 2.0
149 stars 72 forks source link

Latest apt release for ROS Noetic on Ubuntu 20.04 has int64 id instead of string id #56

Closed r0gi closed 3 years ago

r0gi commented 3 years ago

OS: Ubuntu 20.04 (focal)

Installed a fresh ROS Noetic following the official instructions but installing vision_msgs via apt doesn't install the latest version

sudo apt update
sudo apt install ros-noetic-vision-msgs

apt show ros-noetic-vision-msgs

Package: ros-noetic-vision-msgs
Version: 0.0.1-1focal.20210112.080130
Priority: optional
Section: misc
Maintainer: Adam Allevato <REDACTED>
Installed-Size: 833 kB
Depends: ros-noetic-geometry-msgs, ros-noetic-message-generation, ros-noetic-message-runtime, ros-noetic-sensor-msgs, ros-noetic-std-msgs
Download-Size: 48.2 kB
APT-Manual-Installed: yes
APT-Sources: http://packages.ros.org/ros/ubuntu focal/main amd64 Packages
Description: Messages for interfacing with various computer vision pipelines, such as object detectors.

rosmsg show vision_msgs/ObjectHypothesisWithPose

int64 id
float64 score
...

Which is different to the latest Noetic API reference, which shows the correct string id.

SteveMacenski commented 3 years ago

I'm not entirely sure - @Kukanani? I see 1.0.0 released last off noetic but I don't see 1.0.0 in the package.xml tags of the noetic branch. I still see 0.0.1 in rosdistro https://github.com/ros/rosdistro/blob/master/noetic/distribution.yaml#L8048 so that 1.0.0 wasn't bloomed up?

Kukanani commented 3 years ago

@r0gi sorry for the confusion.

There has not been a release since 0.0.1 in ROS1. The commit tree is fairly messy; I think that most changes that are on noetic-devel should be in master instead. See screenshot from gitk at the end of this post.

I would bloom up another release for noetic but this will represent a breaking change for current package users. I seem to remember it being a general policy not to push breaking changes to message definitions after a ROS distro enters regular usage. @SteveMacenski is this still a good rule of thumb?

I propose the following:

Screenshot from 2021-04-14 09-15-50

SteveMacenski commented 3 years ago

@SteveMacenski is this still a good rule of thumb?

Still the right answer -- was that released before we moved this into ros-perception? I suppose I missed that. We should probably revert any changes from that release in the noetic branch if we're not going to re-release with changes.

I suggest to delete master, what value does it really provide if we don't release it and the changes between 0.0.1 and 1.0.0 are mostly superficial? We're not going to be able to change noetic / ROS1 distros so maintaining that master branch doesn't really do anything since it won't go anywhere in the future. The ROS2 branch is the "new master" w.r.t. changes being actually used in future distros

Kukanani commented 3 years ago

was that released before we moved this into ros-perception?

Yes, I believe so.

I suggest to delete master, what value does it really provide if we don't release it and the changes between 0.0.1 and 1.0.0 are mostly superficial?

Yes, I'll remove. At the time, I was expecting at least one more ROS1 release. It doesn't serve much purpose now. I guess that master could potentially be used to ease the transition from ROS1 to ROS2, or to help fix compatibility issues like #39 for all the changes we recently made to the package. In other words, it could be the "ros1 version of the ros2 message definitions". However this is very confusing and would require additional maintenance effort going forward, so I think it's best to just remove.

Kukanani commented 3 years ago

master is no more.

SteveMacenski commented 3 years ago

It has been reverted

r0gi commented 3 years ago

Thank you, could you also trigger an update for the API reference so that it is consistent with the noetic release? From the linked page it says "autogenerated on Tue, 21 Jul 2020 03:42:48" so it is still showing the old type.

Kukanani commented 3 years ago

I don't think I have the ability to update the API reference. Looking at the dates, it seems that the last autogeneration was a few days after the last commits were pushed to noetic-devel. Maybe we just need to wait a few more days for the changes to be picked up?

mintar commented 1 year ago

In rospy (ROS 1), you can set any value to any field. You could also do crazy stuff like:

vision_msgs.ObjectHypothesis(id=[1, 2, None, {"foo": 1}, "bar"])

... and it works (but it shouldn't). BUT: Once you try to publish that message, you'll get an error.

You can test whether a field is the correct type with this little function I wrote:

def serialize_deserialize(message):
    """
    Serialize and then deserialize a message. This simulates sending a message
    between ROS nodes and makes sure that the ROS messages being tested are
    actually serializable, and are in the same format as they would be received
    over the network. In rospy, it is possible to assign an illegal data type
    to a message field (for example, `message = String(data=42)`), but trying
    to publish this message will throw `SerializationError: field data must be
    of type str`. This method will expose such bugs.
    """
    from io import BytesIO

    buff = BytesIO()
    message.serialize(buff)
    result = message.__class__()  # create new instance of same class as message
    result.deserialize(buff.getvalue())
    return result

So in short: The id type must be an integer.

zkytony commented 1 year ago

Hi @mintar Thanks for the reply. I soon realized that and removed my post.

To people who read this later, I asked a question where I can create ObjectHypothesis with a string id field, even though the API reference says the field is integer.

I am moving forward with using the integer id.