-
Notifications
You must be signed in to change notification settings - Fork 129
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 detailed pedestrian description to MovingObject #710
Add detailed pedestrian description to MovingObject #710
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good to me in general, and I can imagine using it. Only two minor comments from my side.
I might have some feedback here. I will get back here! |
Is this superseding #305 ? |
This might be relevant. Did you consider their definitions on skeletal joint positions? |
15.05.WG ASAM OSI | Other Models | Focus: Enhanced Pedestrian Modeling:
|
Two comments regarding the current patch-set:
I want to elaborate a bit on the second point: Comments/Objections? |
Hi Jakob,
Best regards, Edit: As stated above by @tbleher, it looks like the PedestrianAttributes Message is not yet referenced anywhere. Ít should be added to the MovingObject, obviously. |
WG Other Models:
|
osi_object.proto
Outdated
// | ||
// Reference System is the root, defined by bbcenter_to_root | ||
// (\c PedestrianAttributes::bbcenter_to_root). | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is removed.
osi_object.proto
Outdated
// Bone defines one of the thighs. | ||
// | ||
TYPE_UPPER_LEG_L = 14; | ||
// Bone defines one of the thighs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in an empty line.
osi_object.proto
Outdated
// | ||
optional bool missing = 5; | ||
|
||
// The type of the bone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor stuff.
CCB Review, 19.06.23: |
osi_object.proto
Outdated
// root point is used, all bones between that bone and the root also | ||
// need to be defined in order to create a complete chain! | ||
// | ||
// If information about bones are missing, they may be left empty. A complete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short-notice review: I'd rephrase or even remove "A complete set of bones for the entire skeleton cant't be expected!". This is potentially irritating/misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted this sentence
Signed-off-by: Jakob Peintner <jakob.peintner@thi.de>
Fix typos in OSI_SkeletonNamingConvention.jpg and change the incorrect reference in osi_object.proto from BaseMoving::position to BaseMoving::orientation. Signed-off-by: Jakob Peintner <jakob.peintner@thi.de>
05e5c07
to
ba7805f
Compare
Based on the discussions in the workgroup, Pedestrian Data was reformatted to Pedestrian Attributes. Signed-off-by: Jakob Peintner <jakob.peintner@thi.de> Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
1) Skeleton structure was simplified (omitting fingers, eyes, and jaw); FULL_HAND introduced instead; 2) Coordinate reference was redefined with bbcenter_to_root; 3) Skeleton Point was renamed to Bone for clarity; 4) Length attribute added to the bone; Signed-off-by: Jakob Peintner <jakob.peintner@thi.de>
Signed-off-by: Jakob Peintner <jakob.peintner@thi.de>
Signed-off-by: Jakob Peintner <jakob.peintner@thi.de>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor improvements that might be needed.
ba7805f
to
0cf4e9f
Compare
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
Add a description
Related to WP12 in other models workgroup. For exchanging motion capture data of pedestrians (or humans in general) between simulators, we need a more detailed description of pedestrians. This is based on similar skeleton structures used for character rigging in CARLA, Unity, Unreal, etc.
Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board: