Skip to content

add tracking ID to the Detection Message#19

Merged
Kukanani merged 6 commits intoros-perception:kinetic-develfrom
OUXT-Polaris:master
Jul 19, 2019
Merged

add tracking ID to the Detection Message#19
Kukanani merged 6 commits intoros-perception:kinetic-develfrom
OUXT-Polaris:master

Conversation

@hakuturu583
Copy link
Copy Markdown
Contributor

@hakuturu583 hakuturu583 commented May 30, 2019

I want to add tracking infomation I described in this Issue.
#18

@hakuturu583 hakuturu583 marked this pull request as ready for review May 30, 2019 10:13
@hakuturu583 hakuturu583 changed the title add tracking ID to the Detection add tracking ID to the Detection Message May 30, 2019
@hakuturu583
Copy link
Copy Markdown
Contributor Author

Does anyone check this PR?

Copy link
Copy Markdown
Collaborator

@Kukanani Kukanani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

bool is_tracking

# Unique ID which was set by tracker.
uuid_msgs/UniqueID tracking_id No newline at end of file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the comments to the following, for better readability? (on both message definitions)

# If true, this message contains object tracking information.

# ID used for consistency across multiple detection messages.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename tracking_id to object_id because it can also be used in other contexts, where the result is not generated from a tracker.
Moreover, I would remove the is_tracking field. This can be easily derived from the context, e.g. the publishing node or that tracking_id/object_id is different from 0 or "".

Copy link
Copy Markdown
Contributor Author

@hakuturu583 hakuturu583 Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mintar I disagree with you. In my opinion, detector cannot set object ID because they did not do time series processing.

I think this will be a magic number and it will become a technical debut in the future.

e.g. the publishing node or that tracking_id/object_id is different from 0 or "".

So, I also disagree with this opinion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kukanani OK! This modification is good!
So I will update my comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hakuturu583 I'm not too adamant about removing is_tracking. What do you mean by that a a detector, in this instance a tracker, does not do time series processing.

By tracker I understand a program that tracks one object across multiple frames, e.g. a person walking in the image from left to right. The tracker retains state (bounding box in last frame + features) to find the object in the next frame. So the tracker does indeed do time series processing on the series of frames.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I just don't understand the meaning of tracking_id. Assume I track a person walking from left to right in the image in multiple frames: Does each message get the same tracking_id because it is the same person as outputted by the tracker?

bool is_tracking

# Unique ID which was set by tracker.
uuid_msgs/UniqueID tracking_id No newline at end of file
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comments, see note above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!

@Kukanani Kukanani self-assigned this Jul 8, 2019
@Kukanani Kukanani mentioned this pull request Jul 8, 2019
@Kukanani
Copy link
Copy Markdown
Collaborator

Kukanani commented Jul 9, 2019

After further reflection (and as pointed out in #17), I think a string would be a more flexible representation to use. A string could be used to store a UUID if desired (using any UUID generator), but wouldn't require additional dependencies from everyone who wants to use this feature.

@mistermult
Copy link
Copy Markdown
Contributor

After further reflection (and as pointed out in #17), I think a string would be a more flexible representation to use. A string could be used to store a UUID if desired (using any UUID generator), but wouldn't require additional dependencies from everyone who wants to use this feature.

A string needs more overhead because the length has to be saved etc. We should use the same type for id and tracking_id. Both are foreign keys into a collection of classes/objects. So both of them should be int64 or both of them should be strings.

Using strings for classes is even better: This would make is easy to just enter the class name without lookup, enter WikiData IDs, e.g. Q470112, or URLs (keyword: linked data).

@hakuturu583
Copy link
Copy Markdown
Contributor Author

@mintar Do you think how to treat same class objects at the same scene??

@Kukanani Kukanani changed the base branch from master to kinetic-devel July 19, 2019 22:01
@Kukanani
Copy link
Copy Markdown
Collaborator

I've switched the tracking_id field to a string to avoid the UUID dependency. Of course, if you'd like, you can still store a string representation of the UUID, but this is more flexible.

Thanks for the PR!

@Kukanani Kukanani merged commit 0d20a43 into ros-perception:kinetic-devel Jul 19, 2019
@hakuturu583
Copy link
Copy Markdown
Contributor Author

@Kukanani Please revert this commit(3a38cdd) and retrun to UUID description.
Let's discuss in #18

@hakuturu583
Copy link
Copy Markdown
Contributor Author

@Kukanani
Flexible representation shold not be used in designing ROS modules.
It causes problem while connecting modules.

Kukanani added a commit that referenced this pull request Oct 4, 2019
Kukanani added a commit that referenced this pull request Oct 4, 2019
Kukanani added a commit that referenced this pull request Apr 16, 2021
SteveMacenski pushed a commit that referenced this pull request Apr 16, 2021
* Revert "add tracking ID to the Detection Message (#19)"

This reverts commit 0d20a43.

* Revert "Convert id to string (#22)"

This reverts commit 866d02f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants