Add new message for Bounding Box sub-sections #695
Conversation
ndunningbmw
changed the title
ThomasNaderBMW
added
the
ReadyForCCBReview
ThomasNaderBMW
removed
the
ReadyForCCBReview
osi_common.proto
Outdated
| // | ||
| optional string model_reference = 5; | ||
|
|
||
| // Definition of different types of object contained within the bounding box |
There was a problem hiding this comment.
Is there a reference for this list or is it a result of a working-group brainsorming?
If there are sources, I qould like to see them referenced here.
osi_common.proto
Outdated
| // either that of a \c MovingObject or \c StationaryObject . | ||
| // | ||
| // The parent frame of the \c BoundingBox is not relative to the parent object | ||
| // it is associated to, but in the same parent frame as the parent object. |
There was a problem hiding this comment.
I don't understand this sentence, could it be reworded?
There was a problem hiding this comment.
What's actually meant is that a sub bounding box is not relative to its parent bounding box but rather to the parent frame of its parent bounding box. It is in the same frame as its parent object. E.g. if the main bounding box is in global coordinates, the sub bounding box is as well.
But I definitely agree, that it is hard to understand. Could also be extended by an example.
There was a problem hiding this comment.
Suggestion:
The parent frame of a bounding box subsection is identical to the parent frame of its associated/parent object (e.g. MovingObject/BaseMoving).
We could also leave it to the higher level field (bounding_box_section) to define the parent frame and keep anything related to "subsection"/"bounding box section" out of the BoundingBox description. What do you think, @thempen? If we ever wanted to use this BoundingBox message for things other than subsections (e.g. main bounding boxes), we would have to change this again.
osi_common.proto
Outdated
| // The bounding box sections can include side mirrors, cargo, etc. for | ||
| // vehicles, as well as body-part sections for pedestrians. | ||
| // | ||
| // \note When one or more \c BoundingBox s are associated to a | ||
| // \c BaseMoving , the expectation is that all sections are contained | ||
| // within the bounds of the \c #dimension . |
There was a problem hiding this comment.
Saying that bounding box sections can include side mirrors (also cargo) here but then saying they might not be included within the bounding box dimensions in the respective enum description (see line 459) seems kind of ambiguous and confusing to me.
Shouldn't we be clear about if we want them to be included or not? Otherwise some people include side mirrors/cargo, others don't...
I think the main questions are:
- Would it break backwards compatibility if we include side mirrors in the bounding box?
- If yes, do we want to change this for 4.0?
Based on that I would rephrase the descriptions to explain why it is as it is and maybe also state the goal for the next major release (if there is any).
|
There should also be harmonization with the model structure defined in OpenMaterial @LudwigFriedmann. |
|
Special Meeting "OSI: Sub-Bounding-Boxes", 15.05.23:
==> Group gives ok. Topic goes on in Other Models. |
This PR proposes the addition of bounding box sub-sections to BaseMoving and BaseStationary objects, to help sub-divide and classify different parts of the overall object structure.
Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:
If you can’t check all of them, please explain why.
If all boxes are checked or commented and you have achieved at least one positive review, you can assign the label ReadyForCCBReview!
Signed-off-by: Nicholas Dunning Nicholas.Dunning@bmw.de
Original PR: #685