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_internal_planning_msgs): adaption to autoware_motion_utils #45

Merged

Conversation

NorahXiong
Copy link
Contributor

@NorahXiong NorahXiong commented Jan 17, 2025

Description

Adaption for porting autoware_motion_utils from autoware.universe to autoware.core:

  1. add msg required by autoware_motion_utils
  2. fix PathPoint type error cause by duplicate definition in repo autoware_msgs and autoware_internal_msgs (error source)

How was this PR tested?

Notes for reviewers

None.

Effects on system behavior

None.

Copy link

github-actions bot commented Jan 17, 2025

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@NorahXiong NorahXiong changed the title Adaption to autoware motion utils Adaption to autoware_motion_utils Jan 17, 2025
@NorahXiong NorahXiong requested review from youtalk and removed request for youtalk January 17, 2025 10:02
@NorahXiong NorahXiong added the run:build-and-test-differential Run build-and-test-differential workflow label Jan 17, 2025
@NorahXiong NorahXiong changed the title Adaption to autoware_motion_utils feat(autoware_internal_planning_msgs): adaption to autoware_motion_utils Jan 17, 2025
@mitsudome-r
Copy link
Member

mitsudome-r commented Jan 17, 2025

@NorahXiong
Hi, could you split these two items into separate PRs? We should merge the second item right away, but I would like to wait a little for the first item.

  1. add msg required by autoware_motion_utils
  2. fix PathPoint type error cause by duplicate definition in repo autoware_msgs and autoware_internal_msgs (error source)

I am asking TIER IV's planning team to extract out the functions from the autoware_motion_utils package that uses the messages that you are trying to add in this PR because they are not necessary for Autoware Core. If it goes well, we might not need those messages to be added in this repo. Therefore, we can wait for the planning team's effort before merging the first feature.

However, I would like to merge the fix for PathPoint regardless of the planning team's effort because that would impact other packages that we are planning to port to Autoware Core. Therefore, we can split the PR and merge that first.

@mitsudome-r
Copy link
Member

mitsudome-r commented Jan 17, 2025

I'm also a bit concerned about deleting the message(PathPoint.msg) that was added in 1.4.0. (Sorry, I should have also been careful when I reviewed #45 in the first place)

We don't have any modules that's using the message yet, and we also haven't pushed 1.4.0 to the release repo either so I think deleting it would have no issue to anyone. However, 1.4.0 and the next version will no longer be compatible if we merge this PR.

@youtalk If we have to merge this, should we release the next version as 2.0 in this case?

@youtalk
Copy link
Member

youtalk commented Jan 18, 2025

Fortunately I didn't release 1.4.0 via bloom yet. So we can still continue to use 1.X.X.
https://github.com/ros2-gbp/autoware_internal_msgs-release

@NorahXiong
Copy link
Contributor Author

@NorahXiong Hi, could you split these two items into separate PRs? We should merge the second item right away, but I would like to wait a little for the first item.

  1. add msg required by autoware_motion_utils
  2. fix PathPoint type error cause by duplicate definition in repo autoware_msgs and autoware_internal_msgs (error source)

I am asking TIER IV's planning team to extract out the functions from the autoware_motion_utils package that uses the messages that you are trying to add in this PR because they are not necessary for Autoware Core. If it goes well, we might not need those messages to be added in this repo. Therefore, we can wait for the planning team's effort before merging the first feature.

However, I would like to merge the fix for PathPoint regardless of the planning team's effort because that would impact other packages that we are planning to port to Autoware Core. Therefore, we can split the PR and merge that first.

@mitsudome-r @youtalk Done. The second change has been moved to #46.

@NorahXiong NorahXiong force-pushed the adaption-to-autoware_motion_utils branch from f15d927 to 4595e77 Compare January 19, 2025 06:50
@NorahXiong NorahXiong force-pushed the adaption-to-autoware_motion_utils branch from 7ce7e87 to 52a1b1b Compare January 19, 2025 07:06
Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

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

LGTM

@youtalk youtalk enabled auto-merge (squash) January 23, 2025 06:23
@youtalk youtalk merged commit cb0de87 into autowarefoundation:main Jan 23, 2025
9 checks passed
@NorahXiong
Copy link
Contributor Author

@NorahXiong Hi, could you split these two items into separate PRs? We should merge the second item right away, but I would like to wait a little for the first item.

  1. add msg required by autoware_motion_utils
  2. fix PathPoint type error cause by duplicate definition in repo autoware_msgs and autoware_internal_msgs (error source)

I am asking TIER IV's planning team to extract out the functions from the autoware_motion_utils package that uses the messages that you are trying to add in this PR because they are not necessary for Autoware Core. If it goes well, we might not need those messages to be added in this repo. Therefore, we can wait for the planning team's effort before merging the first feature.

However, I would like to merge the fix for PathPoint regardless of the planning team's effort because that would impact other packages that we are planning to port to Autoware Core. Therefore, we can split the PR and merge that first.

Hi, @mitsudome-r and @youtalk, I noticed the PR is merged. Will TIER IV's team continue working on extracting the functions using these messages? If so, I will wait until the work is finished. Otherwise, I will try to update core #171 right now.

@youtalk
Copy link
Member

youtalk commented Jan 24, 2025

Ah, I might have merged it prematurely. I wasn’t keeping up with the discussion above.
@mitsudome-r We need to revert this?

I'm sorry, this is about autoware_motion_utils porting. This PR seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run:build-and-test-differential Run build-and-test-differential workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants