Skip to content

Move assigned_lane_id in osi_object.proto to VehicleClassification #368

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 12, 2021
Merged

Move assigned_lane_id in osi_object.proto to VehicleClassification #368

merged 5 commits into from
Feb 12, 2021

Conversation

0815-code
Copy link
Contributor

Reference to a related issue in the repository

Add a reference to a related issue in the repository.

Add a description

Add the assigned lane id to the vehicle classification as it is the case with other entities (e.g. traffic signs)

Some questions to ask:
What is this change?
What does it fix?
Add the assigned lane id to the vehicle classification as it is the case with other entities (e.g. traffic signs)
Is this a bug fix or a feature? Does it break any existing functionality or force me to update to a new version?
It is a bug fix. Fields got altered, therefore minor version upgrade.

How has it been tested?
Documentation was build

Vehicle Classification without assigned lane id
image

Vehicle Classification with assigned lane id
image

Mention a member

@jdsika @ThomasNaderBMW

Check the checklist

  • My code and comments follow the style guidelines and contributors guidelines of this project.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests / travis ci pass locally with my changes.

@0815-code 0815-code added this to the v3.2.0 milestone Jan 15, 2020
@0815-code 0815-code requested a review from jdsika January 15, 2020 13:10
@jdsika jdsika modified the milestones: v3.2.0, v4.0.0 Feb 19, 2020
@0815-code
Copy link
Contributor Author

0815-code commented Mar 22, 2020 •
edited
Loading

@jdsika @pmai Would it be an option to only copy the assigned_lane_id field for now to the vehicle classification message in order to be downwards compatible and delete the assigned_lane_id then later?

@0815-code 0815-code requested a review from pmai March 22, 2020 20:36
@pmai
Copy link
Contributor

pmai commented Mar 22, 2020

That would definitely be a better option for now, and we could deprecate the original field (or even delete without renumbering). That said, I'd also think that this should merge the percentage fields in DetectedMovingObject into this new field as well, e.g. something like:

repeated Identifier assigned_lane_id = 5;
repeated double assinged_lane_percentage = 6;

@0815-code
Copy link
Contributor Author

0815-code commented Mar 23, 2020 •
edited
Loading

@pmai ok, have reverted the last commit and left the lane id in vehicle classification.
What is the assigned lane percentage? Where can i find that? Or it doesnt exist yet?

repeated double assinged_lane_percentage = 6; still missing

@pmai
Copy link
Contributor

pmai commented Apr 29, 2020

These are the changes I had in mind.

However looking at the overall situation it seems to me that assigned lanes also should apply to non-vehicle moving objects potentially (e.g. pedestrians, animals, ...); which might have been the reason why the fields were not initially put into the VehicleClassification.

So a cleaner solution might be to introduce a MovingObjectClassification, and move those fields there...

Any opinions on this?

@kmeids kmeids modified the milestones: (REMOVE) ASAM OSI x.x.x, V3.3.0 Oct 28, 2020
@kmeids kmeids added FeatureRequest Proposals which enhance the interface or add additional features. Harmonisation The Group in the ASAM development project working on harmonisation with other standards. SensorModeling The Group in the ASAM development project working on sensor modeling topics. Bug Problems in the build system, build scripts, etc or faults in the interface. and removed FeatureRequest Proposals which enhance the interface or add additional features. labels Oct 28, 2020
@0815-code
Copy link
Contributor Author

@pmai. I agree but I would suggest to make this bugfix now and do a complete rework later (for osi v4 maybe).

What do you think @pmai, @kmeids ?

@clemenshabedank clemenshabedank added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Dec 2, 2020
@clemenshabedank
Copy link
Contributor

Discussed in WP "OSI as a measurement file" from @ThomasSchloemicherAVL (2020-12-02) that it is Ready for CCB review.

@clemenshabedank
Copy link
Contributor

I have thought about this topic again. Isn't the information semantically different depending on where it is located. So currently it is inside DetectedMovingObject which means that it is the output of the sensor model (so how the sensors see the world, which might be different to Ground Truth). VehicleClassification is part of GroundTruth which is provided by the environment simulator. So we could have both!? @ThomasSchloemicherAVL can you check if it is part of ISO-23150? If yes, I would not depreciate it in DetectedMovingObject in the major release. I think it makes sense to also include it in VehicleClassification.

@clemenshabedank
Copy link
Contributor

These are the changes I had in mind.

However looking at the overall situation it seems to me that assigned lanes also should apply to non-vehicle moving objects potentially (e.g. pedestrians, animals, ...); which might have been the reason why the fields were not initially put into the VehicleClassification.

So a cleaner solution might be to introduce a MovingObjectClassification, and move those fields there...

Any opinions on this?

Or we introduce PedestrianClassification and AnimalClassification, in case the MovingObject is one of those? This would be backward compatible and fit the current logic.

@kmeids
Copy link

kmeids commented Dec 9, 2020 •
edited
Loading

Output from CCB meeting - 09.12.2020

Actions:

  1. Further technical discussion should be done on this topic, by @ThomasSchloemicherAVL @clemenshabedank and @pmai.
  2. Discuss in the Architecture Group meeting as well.

@kmeids kmeids removed the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Dec 9, 2020
@HendrikAmelunxen HendrikAmelunxen added TrafficParticipants The group in the ASAM development project working on traffic participants. and removed Harmonisation The Group in the ASAM development project working on harmonisation with other standards. labels Jan 21, 2021
@clemenshabedank
Copy link
Contributor

clemenshabedank commented Jan 25, 2021 •
edited
Loading

I have thought about this topic again. Isn't the information semantically different depending on where it is located. So currently it is inside DetectedMovingObject which means that it is the output of the sensor model (so how the sensors see the world, which might be different to Ground Truth). VehicleClassification is part of GroundTruth which is provided by the environment simulator. So we could have both!? @ThomasSchloemicherAVL can you check if it is part of ISO-23150? If yes, I would not depreciate it in DetectedMovingObject in the major release. I think it makes sense to also include it in VehicleClassification.

This comment of mine is wrong after our discussion and looking into the class diagram again. VehicleClassification is part of SensorView and (in another intepretation) in SensorData, which makes sense for the two fields. So I think it's a good idea to move it. So to recap already your suggestions:

  1. Keep it like it is (Thomas suggestion)
  2. Introduce MovingObjectClassification and put it there (Pierres suggestion)

@ThomasSchloemicherAVL and @pmai what do you think is better?
I think introducing MovingObjectClassification would make it more reusable, and we wouldn't have to double the fields if we introduce e.g. PedestrianClassification in the future, where the fields would also make sense... Should I make the according changes or should we leave the PR as it currently is?

@0815-code
Copy link
Contributor Author

@clemenshabedank I think we should go for @pmai 's suggestion.

@clemenshabedank clemenshabedank removed the SensorModeling The Group in the ASAM development project working on sensor modeling topics. label Feb 4, 2021
@clemenshabedank
Copy link
Contributor

@stefancyliax @pmai: We discussed in @ThomasSchloemicherAVL 's WP that this PR would be readyforCCBReview. We were wondering if just one of us is allowed to do the signoffs for all the commits where it is missing (contributions from Thomas, Pierre and me). Else, could you please help how it would work for each one to do it separatedly for the single commits?

@0815-code
Copy link
Contributor Author

@clemenshabedank mustn't the MovingObjectClassification also be included in DetectedMovingObject?

@pmai
Copy link
Contributor

pmai commented Feb 8, 2021

@clemenshabedank mustn't the MovingObjectClassification also be included in DetectedMovingObject?

More specifically into DetectedMovingObject.CandidateMovingObject, yes...

@0815-code 0815-code mentioned this pull request Feb 8, 2021
Open
@clemenshabedank
Copy link
Contributor

@ThomasSchloemicherAVL and @pmai thanks you are right. I changed it in 3f6db80

I also added the field in MovingObject because the information of the assigend lane id is currently already in the GroundTruth. When it will be depreciated, we have it then under the new submessage. --> b412f89

Does that make sense for you?

kmeids
kmeids approved these changes Feb 11, 2021
View reviewed changes
Copy link

@kmeids kmeids left a comment

Choose a reason for hiding this comment

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

Approved during the CCB meeting 11.02.2021

@kmeids
Copy link

kmeids commented Feb 11, 2021

Output of CCB meeting 11.02.2021
Actions:
1.@pmai to do the merge (all agreed for @pmai to do the sign.off )

@kmeids kmeids added the ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. label Feb 11, 2021
0815-code and others added 5 commits February 12, 2021 18:25
@0815-code @pmai
Move assigned_lane_id in MovingObject to VehicleClassification
f679730 
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai
Add assigned_lane_percentage, add deprecations
d4eaae3 
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai
Introduce MovingObjectClassification in MovingObject
d7fab37 
Add MovingObjectClassification and put assigend_lane_id and
assigned_lane_percentage there to allow for filling those fields
for non-vehicle moving objects

Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai
Add MovingObjectClassification to CandidateMovingObject
373fc41 
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai
Add MovingObjectClassification to MovingObject
97444fa 
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai pmai force-pushed the bugfix/osi_object branch from b412f89 to 97444fa Compare February 12, 2021 17:29
@pmai pmai merged commit 7097103 into master Feb 12, 2021
@pmai pmai deleted the bugfix/osi_object branch February 12, 2021 19:34
@0815-code 0815-code mentioned this pull request Oct 11, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmai pmai pmai approved these changes

@kmeids kmeids kmeids approved these changes

@jdsika jdsika Awaiting requested review from jdsika

Assignees
No one assigned
Labels
Bug Problems in the build system, build scripts, etc or faults in the interface. ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. TrafficParticipants The group in the ASAM development project working on traffic participants.
Projects
None yet
Milestone
V3.3.0
Development

Successfully merging this pull request may close these issues.
6 participants
@0815-code @pmai @clemenshabedank @kmeids @jdsika @HendrikAmelunxen