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

Add new enum Type in vehicle classification for stand-up scooters #676

Conversation

schmidtlorenz
Copy link
Contributor

feat(vehicle classifications): add classification for stand-up scooters, motivated by possible NCAP target type

#### Reference to a related issue in the repository
refers to issue #673

Add a description

This adds a new enum Type in vehicle classification for stand-up or kickboard scooters.

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

@jdsika
Copy link
Contributor

jdsika commented Nov 4, 2022

Please fix build in order to hand this over to the CCB. Can you add the reference to a relevant NCAP document? Maybe even in the description. I think most descriptions would benefit from a little more verbose comments.

@jdsika jdsika added this to the V3.6.0 milestone Nov 4, 2022
@jdsika jdsika added the FeatureRequest Proposals which enhance the interface or add additional features. label Nov 4, 2022
@ThomasNaderBMW
Copy link
Contributor

Jobs are failing because of this: osi_object.proto:871:36: "osi3.MovingObject.VehicleClassification.TYPE_STANDUP_SCOOTER" uses the same enum value as "osi3.MovingObject.VehicleClassification.TYPE_SEMITRACTOR". If this is intended, set 'option allow_alias = true;' to the enum definition.

So Semitractor has already ID 16, you can take 17 e.g. @schmidtlorenz

osi_object.proto Outdated
@@ -865,6 +865,11 @@ message MovingObject
// Vehicle is a wheelchair.
//
TYPE_WHEELCHAIR = 15;

// Vehicle is a stand-up or kickboard scooter, including motorized versions.
// This vehicle type accounts for the following test dummies: https://www.messring.de/de/produkte/aktive-sicherheit/astera-e-scooter/ and https://www.4activesystems.at/4activeps-ekickboard-scooter
Copy link
Contributor

Choose a reason for hiding this comment

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

When you're using references you should comply with the contribution guidelines concerning references. The referenced websites may not be reliably available in the future. Maybe the description in line 868 alone is even enough to know what is meant?

Also, the build tests fail again because there are some additional tabs before the "//"-comment in line 870.

@ThomasNaderBMW ThomasNaderBMW added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Dec 5, 2022
@schmidtlorenz
Copy link
Contributor Author

I removed the links since I could not find persistent versions (e.g. PURL or DOI).

@pmai pmai added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Dec 12, 2022
feat(vehicle classifications): add classification for stand-up scooters, motivated by possible NCAP target type

refers to issue OpenSimulationInterface#673

Signed-off-by: schmidtlorenz <107173594+schmidtlorenz@users.noreply.github.com>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai pmai force-pushed the 673-new-vehicle-classification-stand-up-scooter branch from bfc1538 to d1253f3 Compare December 12, 2022 13:01
Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

CCB 2022-12-12: Merge as-is.

Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest Proposals which enhance the interface or add additional features. Other Models ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants