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: add multi lanelet parser #17

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

StepTurtle
Copy link

Description

Move autowarefoundation/autoware_common#234 to new autoware_lanelet2_extension repository.

This PR add a new library class to facilitate the loading of multiple OSM files in lanelet2_extension. These changes were made to support dynamic lanelet loading.

Related links

Proposal Link

Tests performed

In this video, the map in background loaded with current approach and the white map load new class and cannot see any difference. Also the maps which loaded with new class tested with mission and behavior planner and cannot see any problem.

Video Link

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.

@mitsudome-r
Copy link
Member

I was wondering if we should create another parser for loading multiple files. Are there any reasons for not merging the Lanelets after loading each files one by one?

@StepTurtle
Copy link
Author

I was wondering if we should create another parser for loading multiple files. Are there any reasons for not merging the Lanelets after loading each files one by one?

After call OsmParser::parse and create map object, I did not find a good way to merge map objects. Is there any way to merge map objects?

@YamatoAndo
Copy link

@reviewers
@kosuke55 @soblin @Motsu-san @mitsudome-r @takayuki5168 @youtalk
The review of the related PRs is almost finished.
I would appreciate it if you could proceed with the review of this PR.

@Motsu-san
Copy link
Collaborator

@soblin Could you review and test this PR?

@Motsu-san
Copy link
Collaborator

@soblin Could you review and test this PR?

@soblin I misunderstood. In the parent PR, Maxim-san already has tested the related PR including this PR. So please ignore my request to test.

@Motsu-san
Copy link
Collaborator

Motsu-san commented Aug 29, 2024

I was wondering if we should create another parser for loading multiple files. Are there any reasons for not merging the Lanelets after loading each files one by one?

After call OsmParser::parse and create map object, I did not find a good way to merge map objects. Is there any way to merge map objects?

@mitsudome-r Could you respond to this message?

@YamatoAndo
Copy link

@kosuke55 @soblin @Motsu-san @mitsudome-r @takayuki5168 @youtalk
Review this PR!!

class MultiFileLoader
{
public:
static std::unique_ptr<LaneletMap> loadMap(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you align the function return type to lanelet::LaneletMapPtr just like lanelet::io::load ? Although I guess lanelet::LaneletMapPtr is just an alias of std::unique_ptr (or shared_ptr)

Copy link
Author

Choose a reason for hiding this comment

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

loader.lineStrings_, loader.points_);
}

static std::unique_ptr<LaneletMap> loadMap(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this future related to the differential loading mechanism ? Then can we dynamically add next map while dropping visited map by using the add and removed function to each layers, so we do not need to recreate the entire lanelet map whenever ego passes the boundary ?

https://github.com/fzi-forschungszentrum-informatik/Lanelet2/blob/507033b82d9915f086f2539d56c3b62e71802438/lanelet2_core/include/lanelet2_core/LaneletMap.h#L245-L263

@soblin
Copy link
Collaborator

soblin commented Sep 27, 2024

@StepTurtle

After call OsmParser::parse and create map object, I did not find a good way to merge map objects. Is there any way to merge map objects?

I think my comments answers your question. I believe it is possible to merge/delete map objects in sliding window manner in the context of differential map loading.

Here is my suggestion:

@mitsudome-r
Copy link
Member

@StepTurtle

After call OsmParser::parse and create map object, I did not find a good way to merge map objects. Is there any way to merge map objects?

You can create a function to merge each layer of Lanelet map as mentioned by @soblin's comment.
Something like the following:

// merge map2 to map1
void mergeMap( lanelet::LaneletMapPtr map1, lanelet::LaneletMapPtr map2)
{
  for(auto llt : map2->laneletLayer){
    map1.add(llt);
  }
  for(auto area : map2->areaLayer){
    map1.add(area);
  }
  for(auto reg_elem : map2->regulatoryElementLayer){
    map1.add(reg_elem);
  }
  for(auto poly : map2->polygonLayer){
    map1.add(poly);
  }
  for(auto ls : map2->lineStringLayer){
    map1.add(ls);
  }
  for(auto pt : map2->pointLayer){
    map1.add(pt);
  }
}

(Note that above code is just an image and I haven't tested the code.

@YamatoAndo
Copy link

@soblin @mitsudome-r cc: @StepTurtle
Thank you for stating review.
Please create a new release tag after merging.

Signed-off-by: Barış Zeren <[email protected]>
@StepTurtle
Copy link
Author

Hey @YamatoAndo , sorry about this, but I tried a few things and couldn't quite figure out the points from the last comments. I also haven’t had enough time to work on the lanelet reviews. Could I ask for your help with the this PR? Apologies again for the trouble. 🙏🏽

@YamatoAndo
Copy link

@StepTurtle Hi,

I tried a few things and couldn't quite figure out the points from the last comments

After merging this PR, please create a PR similar to #30. Once that PR is merged, we will create a new release tag.
Since I haven't done this task before, I would like you to collaborate with @soblin and @mitsudome-r to move forward with the process.

@YamatoAndo
Copy link

@kosuke55 @soblin @Motsu-san @mitsudome-r @takayuki5168 @youtalk

I also haven’t had enough time to work on the lanelet reviews. Could I ask for your help with the this PR?

Could you support this PR instead?

@soblin
Copy link
Collaborator

soblin commented Oct 18, 2024

@StepTurtle All right, then I'd like to propose moving your implementation in autoware.universe's
common package for now.
The reason for my suggestion is that this "feature" is experimental and has room for future additional requirements and better implementation. Also it should not be placed under map module because it renders planning dependent on map (relates to this initiative https://github.com/orgs/autowarefoundation/discussions/4661#discussioncomment-9995806 cc : @youtalk ). I am Ok with current implementation !

@YamatoAndo
Copy link

@kosuke55 @soblin @Motsu-san @mitsudome-r @takayuki5168 @youtalk
Does it seem like you can support this?

@mitsudome-r
Copy link
Member

mitsudome-r commented Nov 20, 2024

Let me follow up on this.
@StepTurtle Do you have Discord account? I would like to setup a call to how we should treat this PR.

@xmfcx
Copy link
Contributor

xmfcx commented Nov 26, 2024

@mitsudome-r @StepTurtle what are the actions that you've decided to take on this task?

@StepTurtle
Copy link
Author

@mitsudome-r @StepTurtle what are the actions that you've decided to take on this task?

@mitsudome-r will handle this PR and I guess he will do some updates on code in this week.

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