Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move dummy_perception_msgs.Objects message definition to autoware_msgs #8704

Closed
3 tasks done
YoshiRi opened this issue Sep 2, 2024 · 9 comments · Fixed by #8818, #8828 or tier4/tier4_autoware_msgs#146
Closed
3 tasks done
Assignees
Labels
type:improvement Proposed enhancement

Comments

@YoshiRi
Copy link
Contributor

YoshiRi commented Sep 2, 2024

Checklist

  • I've read the contribution guidelines.
  • I've searched other issues and no duplicate issues were found.
  • I've agreed with the maintainers that I can plan this task.

Description

I suggest to move dummy_perception_msgs.Objects message declaration in dummy_perception_publisher into https://github.com/autowarefoundation/autoware_msgs.
This is related to #8695.

Purpose

We want to remove common folder packages dependency to dummy_perception_publisher and it seems to depend its message named dummy_perception_msgs.Objects.

I think we can move this declaration to autoware_msgs.

Possible approaches

I think there are some approaches:

  1. move dummy_perception_msgs.Objects to autoware_msgs/autoware_perception_msgs
  2. create autoware_msgs/autoware_simulation_msgs and put the message into it

Case 1 needs message name change, while case 2 won't need it.
I prefer the latter, but I want to you guys advise.

Definition of done

Once after message transfer and dependency will be solved.

@YoshiRi
Copy link
Contributor Author

YoshiRi commented Sep 2, 2024

@mitsudome-r @yukkysaito
Could you take a look and give some comments?

@mitsudome-r
Copy link
Member

If those two are the options, I would go with the second option.

However, autoware_msgs is meant to be a place where we keep the core interface of Autoware, ideally with a proper design in Autoware Documentation like this page.

We currently do not have a simulator component at the moment with a concrete design document for interface so it might be better to first define what should be the inputs/outputs for simulation modules first rather than randomly adding a new package as autoware_simulation_msgs.

@YoshiRi
Copy link
Contributor Author

YoshiRi commented Sep 3, 2024

@mitsudome-r
So, you mean

  • We should not add this message to autoware_msgs
  • Instead, create autoware_simulator_msgs repo/package and handle this messages,

right?

@takayuki5168
Copy link
Contributor

@YoshiRi @mitsudome-r
How about moving the tier4_perception_rviz_plugin package or necessary specific plugins inside the package to the dummy_perception_publisher package since those plugins are for the simple_planning_simulator via dummy_perception_publisher in my understanding.

@YoshiRi
Copy link
Contributor Author

YoshiRi commented Sep 4, 2024

Then, how about following alternatives? (Which idea is better?)

  1. move tier4_perception_rviz_plugin to simluator folder. We may rename this package to tier4_dummy_perception_rviz_plugin
  2. move message definition of dummy_perception_publisher to newly created package like autoware_simulation_msgs

I prefer method 1, because it is easier.

cc: @technolojin @hakuturu583

@technolojin
Copy link
Contributor

I created a PR to execute the option 1.

@technolojin
Copy link
Contributor

technolojin commented Sep 10, 2024

@YoshiRi @yukkysaito
Can you review the PR #8818 ?

I think tier4_dummy_object_rviz_plugin could be a better name.

@technolojin
Copy link
Contributor

@yukkysaito
Thank you for your review and corporation.

Here is the steps of PR merges.

  1. Merging new message fix(tier4_simulation_msgs): bring message of dummy object to simulator msgs tier4/tier4_autoware_msgs#146
  2. Adapt to the new message and remove the old message fix(dummy_perception_publisher, tier4_dummy_object_rviz_plugin): separate dummy object msg #8828

@xmfcx
Copy link
Contributor

xmfcx commented Sep 26, 2024

@technolojin @yukkysaito @youtalk -san

This message was also being used by the AWSIM Labs. Using the same RViz2 plugins, you can spawn objects in the AWSIM Labs as well.

@Aysenayilmaz will update to the new messages.

I wanted to let you know to keep in mind if a new change is required for this message 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment