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

feat(autoware_map_msgs): add msg and srv files releated with dynamic lanelet loading #81

Merged

Conversation

StepTurtle
Copy link
Contributor

@StepTurtle StepTurtle commented Jan 30, 2024

Description

Add some msg and srv files will use from dynamic lanelet loading.

Related links

Proposal Link

Tests performed

Notes for reviewers

Interface changes

Effects on system behavior

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@yukkysaito
Copy link
Collaborator

Just a confirmation. 🙏
Does this Pull Request mean that approach3 has been adopted?
https://github.com/orgs/autowarefoundation/discussions/4120

@yukkysaito
Copy link
Collaborator

Thank you for your PR 👍
May I make two requests?

@StepTurtle
Copy link
Contributor Author

StepTurtle commented Feb 19, 2024

Thanks for your reviews @yukkysaito ,

* Also, the interface is very important to the microautonomy architecture, so if autoware_msgs is to be changed, it should be included in [the documentation](https://autowarefoundation.github.io/autoware-documentation/main/design/autoware-interfaces/components/map/). 🙏

Okay, I'll update the documentation ASAP. Do I need to change anything else, or is the page you shared enough for now?

* We are currently trying to deprecate autoware_auto_msgs. Therefore, could you please remove the dependency from this PR? Also, the timing of the merge needs to be adjusted. cc @mitsudome-r
  https://github.com/orgs/autowarefoundation/discussions/3862

I have a question: If I remove the dependency, should I use autoware_msgs/msg/LaneletMapBin? If I switch to this message, it seems like I'll need to make several changes on both the map and planner sides. For example, I'd need to update the planner lanelet subscribers, map_loader publishers, and modify the Lanelet2Map to map binary message function, and so on. Can you confirm if my understanding is correct?

@StepTurtle
Copy link
Contributor Author

StepTurtle commented Feb 19, 2024

Just a confirmation. 🙏 Does this Pull Request mean that approach3 has been adopted? https://github.com/orgs/autowarefoundation/discussions/4120

Yes, we are currently trying to implement third approach.

@yukkysaito
Copy link
Collaborator

yukkysaito commented Feb 26, 2024

Okay, I'll update the documentation ASAP. Do I need to change anything else, or is the page you shared enough for now?

Yes. I think it is sufficient. 👍

I have a question: If I remove the dependency, should I use autoware_msgs/msg/LaneletMapBin? If I switch to this message, it seems like I'll need to make several changes on both the map and planner sides. For example, I'd need to update the planner lanelet subscribers, map_loader publishers, and modify the Lanelet2Map to map binary message function, and so on. Can you confirm if my understanding is correct?

Yes. It is better to use autoware_msgs/msg/LaneletMapBin instead of autoware_auto_msgs.
The situation here is complicated, so it might be better to discuss it in the WG. @mitsudome-r

Copy link

stale bot commented May 4, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the stale label May 4, 2024
@YamatoAndo
Copy link
Contributor

@StepTurtle @mitsudome-r @yukkysaito Hi
I want to align the message types as much as possible between PointCloudMap and LaneletMap.

  1. In Lanelet, the term "Tile" is used, whereas in PointCloud, the term "Cell" is used. I am fine with either, but I would like to unify the term.

  2. There are slight differences in how the message types are structured between the two.

For Lanelet, the structure is as follows:
LaneletMapTileMetaData.msg

int32 tile_id
float64 min_x
float64 max_x
float64 min_y
float64 max_y

And for PointCloud, the structure is:
PointCloudMapCellMetaData.msg

float32 min_x
float32 min_y
float32 max_x
float32 max_y

PointCloudMapCellMetaDataWithID.msg

string cell_id
PointCloudMapCellMetaData metadata

I am fine with either structure, but I would like to unify them.

Do you have any ideas?

@stale stale bot removed the stale label Jun 18, 2024
@YamatoAndo
Copy link
Contributor

@StepTurtle GetDifferentialLanelet2Map.srv corresponds to GetSelectedPointCloudMap.srv in PointCloud.
So, could you please change the name to GetSelectedLanelet2Map.srv?

@StepTurtle StepTurtle force-pushed the feat/add_dynamic_lanelet_loading branch from 141883f to 59a616f Compare June 23, 2024 18:50
@StepTurtle
Copy link
Contributor Author

StepTurtle commented Jun 23, 2024

@YamatoAndo

I changed the service name with the name you suggest (GetSelectedLanelet2Map.srv). 👍🏽


About the aligning the message types:

@YamatoAndo
Copy link
Contributor

@StepTurtle Thank you for the reply and the modification!

@mitsudome-r @yukkysaito Do you have any idaes?

@mitsudome-r
Copy link
Member

I have made the comment here: #81 (comment).

@yukkysaito
Copy link
Collaborator

I have made the comment here: #81 (comment).

I also commented on the same part.

@YamatoAndo
Copy link
Contributor

@yukkysaito @mitsudome-r
Please also answer this comment.

In Lanelet, the term "Tile" is used, whereas in PointCloud, the term "Cell" is used. I am fine with either, but I would like to unify the term.

@yukkysaito
Copy link
Collaborator

I want to align the message types as much as possible between PointCloudMap and LaneletMap.

I think either option is fine, but since the existing one is "cell" and is actually being used, it would be better to use "cell" unless someone else has a strong preference.

@YamatoAndo
Copy link
Contributor

@yukkysaito Thank you for the reply!

@StepTurtle Could you please change it to cell_id?

@StepTurtle StepTurtle force-pushed the feat/add_dynamic_lanelet_loading branch from 59a616f to 6af0340 Compare July 4, 2024 13:02
@StepTurtle
Copy link
Contributor Author

@YamatoAndo

Just changed the tile_id with cell_id

@yukkysaito yukkysaito self-requested a review July 4, 2024 14:50
Copy link
Collaborator

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

LGTM
@mitsudome-r @xmfcx @YamatoAndo Any opinions?

Copy link
Contributor

@YamatoAndo YamatoAndo left a comment

Choose a reason for hiding this comment

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

LGTM!

@StepTurtle StepTurtle force-pushed the feat/add_dynamic_lanelet_loading branch from b780fa2 to d38141c Compare July 24, 2024 15:56
Copy link

stale bot commented Sep 27, 2024

This pull request has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the stale label Sep 27, 2024
@YamatoAndo
Copy link
Contributor

@mitsudome-r @yukkysaito
The review of autowarefoundation/autoware.universe#6241 is almost finished, so could you please merge this PR beforehand?

@stale stale bot removed the stale label Sep 27, 2024
StepTurtle and others added 12 commits September 29, 2024 15:35
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
Signed-off-by: Barış Zeren <bzeren1819@gmail.com>
@xmfcx xmfcx force-pushed the feat/add_dynamic_lanelet_loading branch from d38141c to 745dee1 Compare September 29, 2024 12:35
@xmfcx xmfcx enabled auto-merge (squash) September 29, 2024 12:36
@xmfcx xmfcx merged commit 2ae076e into autowarefoundation:main Sep 29, 2024
12 checks passed
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.

6 participants