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 detailed pedestrian description to MovingObject #710

Merged

Conversation

Peintnerer
Copy link
Contributor

@Peintnerer Peintnerer commented Feb 10, 2023

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:

Copy link
Contributor

@tbleher tbleher left a 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.

osi_object.proto Outdated Show resolved Hide resolved
osi_object.proto Outdated Show resolved Hide resolved
osi_object.proto Outdated Show resolved Hide resolved
osi_object.proto Outdated Show resolved Hide resolved
@jdsika
Copy link
Contributor

jdsika commented Feb 27, 2023

I might have some feedback here. I will get back here!

@jdsika jdsika added OpenMSL Required to enable sub-libraries in OpenMSL. Concept An issue that is being detailed out through expert discussion and offline work labels Mar 3, 2023
@jdsika jdsika added this to the V3.6.0 milestone Mar 3, 2023
@jdsika
Copy link
Contributor

jdsika commented Mar 23, 2023

Is this superseding #305 ?

@ClemensLinnhoff
Copy link
Contributor

This might be relevant. Did you consider their definitions on skeletal joint positions?

osi_object.proto Outdated Show resolved Hide resolved
@ThomasNaderBMW
Copy link
Contributor

ThomasNaderBMW commented May 15, 2023

15.05.WG ASAM OSI | Other Models | Focus: Enhanced Pedestrian Modeling:

  • Enum instead of string for skeleton joints
  • Level of detail: Approach is that just the joints are used that are sent (can be identified by the enum). Rule from root till depeest skeleton point (regarding the tree) all skeleton joints must be sent, so complete chain till that point.
  • Will it be part of osi_detectedobject.proto? Depending on that we have to think about if it is vehicle_attributes or vehicle_classification (used in detectedobject)
  • Skeleton Data is enough and can be used in pedestrian_attributes/classification. Pedestrian Data not necessary.
  • Pedestrian_attributes is preferred.

osi_object.proto Outdated Show resolved Hide resolved
@tbleher
Copy link
Contributor

tbleher commented Jun 12, 2023

Two comments regarding the current patch-set:

  • It seems like the new PedestrianAttributes message is not yet referenced anywhere, so currently no pedestrian can be described using this new message. I think it should be added at least to MovingObject, so that the values can be returned from TrafficUpdate (which contains MovingObject), and so that the attributes are part of the GroundTruth.
  • It would be great to have also have some more coarse-grained values, like the current viewing direction.

I want to elaborate a bit on the second point:
The current proposal seems suited for transfering the actual movement of a pedestrian between different systems. However, not all models work with this level of detail. E.g. the open source OSIPedestrian model, which was created as part of the SETLevel project, only works on the bounding box level. 3D animation is done afterwards, e.g. using Unreal. This works fine in practice. However, currently the OSIPedestrian model cannot provide semantic information, e.g. where the pedestrian is currently looking at. This information has to come from OSIPedestrian, since it depends on the situation (example: if the pedestrian is waiting to cross a busy street, it has to look left and right. This semantic info should be provided by the model, and not later be added e.g. in Unreal). However, OSIPedestrian does not do complete animation, since that means it would have to add the whole logic for pedestrian animation inside the model (which is much better done afterwards in Unreal or Unity).
The current patch doesn't really allow a model like OSIPedestrian to add a simple info like e.g. the head direction, without knowing the rest of the geometry of the pedestrian. I propose to add this semantic information separately (and am willing to submit a patch for this).

Comments/Objections?

@LudwigFriedmann
Copy link

LudwigFriedmann commented Jun 13, 2023

Hi Jakob,
just some thoughts:

  • As basis for the enum "Type", the following declarative nomenclature may be used:
  • "SkeletonPoint"s should be renamed to "Bone"s. In the corresponding documentation, it should be clarified that position/orientation of the bone end closer to the semantic origin of the hiearchical bone structure is specified. Further, a float parameter "length" should be introduced in order to cope with potential offsets.
  • A reference coordinate frame should be specified, which is defined relative to the geometric center of the bounding box of the undeflected model (e.g. "bbcenter_to_root"). The x-axis of this coordinate frame should be perpendicular to the bounding box front face, pointing forwards. The z-axis should be perpendicular to the x-axis, pointing upwards. Finally, the y-axis should complete the right-handed coordinate frame. It should be clarified that all bone positions/orientations are specified relative to this coordinate frame.
  • The "height" parameter is redundant and should be removed.
  • Completeness of the bones should not be mandatory. While there shouldn't be gaps/holes in the bone hierarchy, the hierarchy may end early, i.e. not specifying a limb to its full extent.
  • In order to explicitly specify missing limbs, a binary flag "missing" could be introduced for every SkeletonPoint/Bone.
  • Parameters "side" and "number" could be removed. Instead, the full set of bones should be enlisted in the "Type" enum.
  • Introduction of abstract states, e.g. walking, waving, etc. should be postponed. Instead, harmonization with ISO 23150 states should be considered for future releases.

Best regards,
Ludwig

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.

@ThomasNaderBMW
Copy link
Contributor

WG Other Models:

  • Fix Build Errors
  • Add Hirarchy-Picture
  • Can be reviewed by CCB.

@ThomasNaderBMW ThomasNaderBMW added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jun 19, 2023
osi_object.proto Outdated
//
// Reference System is the root, defined by bbcenter_to_root
// (\c PedestrianAttributes::bbcenter_to_root).
//
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this line

Copy link
Contributor Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line.

Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

bone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@ThomasNaderBMW ThomasNaderBMW left a comment

Choose a reason for hiding this comment

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

Some minor stuff.

@ThomasNaderBMW
Copy link
Contributor

CCB Review, 19.06.23:
Can be merged, but minor stuff (Comments, DCO, correct Builds) has to be fixed till Thursday 9 AM

@ThomasNaderBMW ThomasNaderBMW 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 Jun 19, 2023
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
Copy link

@LudwigFriedmann LudwigFriedmann Jun 19, 2023

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.

Copy link
Contributor Author

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>
@pmai pmai force-pushed the WP12_add_pedestrian_model branch from 05e5c07 to ba7805f Compare June 21, 2023 16:04
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>
Peintnerer and others added 4 commits June 22, 2023 06:52
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>
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.

Minor improvements that might be needed.

osi_object.proto Outdated Show resolved Hide resolved
osi_object.proto Outdated Show resolved Hide resolved
osi_object.proto Outdated Show resolved Hide resolved
osi_object.proto Outdated Show resolved Hide resolved
osi_object.proto Show resolved Hide resolved
osi_object.proto Outdated Show resolved Hide resolved
@pmai pmai force-pushed the WP12_add_pedestrian_model branch from ba7805f to 0cf4e9f Compare June 22, 2023 05:10
osi_object.proto Outdated Show resolved Hide resolved
osi_object.proto Outdated Show resolved Hide resolved
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai pmai merged commit a88704d into OpenSimulationInterface:master Jun 22, 2023
4 checks passed
@pmai pmai changed the title Draft for discussion: Add detailed pedestrian description to MovingObject Add detailed pedestrian description to MovingObject Jun 22, 2023
@jdsika jdsika mentioned this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept An issue that is being detailed out through expert discussion and offline work OpenMSL Required to enable sub-libraries in OpenMSL. 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.

None yet

9 participants