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 orientation rate to PedestrianAttributes::Bone #769

Merged

Conversation

ClemensLinnhoff
Copy link
Contributor

@ClemensLinnhoff ClemensLinnhoff commented Jan 25, 2024

Reference to a related issue in the repository

#747

Add a description

Add orientation rate to the bone class. The translatory velocity of the bones can be calculated with the orientation rate of the parent bones.

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

@PhRosenberger PhRosenberger added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jan 25, 2024
@pmai
Copy link
Contributor

pmai commented Jan 29, 2024

CCB 2024-01-29: Should be reviewed by @Peintnerer and @thempen, especially if also velocity should be added, as there is no directly specified relationship between position changes and hierarchical orientation_rate since we have no specified kinematic model (positions and hence gaps between bones can vary over time depending on modeling approaches). Therefore it might be a good idea to include velocity to provide all of the information the internal model has to consumers. To be re-revied at next CCB.

@ClemensLinnhoff
Copy link
Contributor Author

ClemensLinnhoff commented Jan 30, 2024

We thought about it again and came to the conclusion, that also adding the translatory velocity makes sense, mainly because the shoulders and legs are disconnected from the spine. So there can be independent translatory movement.
And as you said, if positions and bone lengths can theoretically also vary over time, we definitely need it.

I added the velocity in the proto file as a suggestion.

Copy link
Contributor

@Peintnerer Peintnerer left a comment

Choose a reason for hiding this comment

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

I can definitely see that having information about the bone's velocity and orientation rate makes sense. This is probably dependent on the modeling approach or motion capture method used, but I can imagine a few applications in which this might come in handy.

@pmai
Copy link
Contributor

pmai commented Feb 12, 2024

CCB 2024-02-12: Decision merge as-is.

ClemensLinnhoff and others added 3 commits February 13, 2024 09:35
Signed-off-by: ClemensLinnhoff <clemens.linnhoff@persival.de>
Signed-off-by: ClemensLinnhoff <clemens.linnhoff@partner.bmw.de>
Signed-off-by: ClemensLinnhoff <clemens.linnhoff@partner.bmw.de>
@pmai pmai force-pushed the 747-add-orientation-rate-to-bone-in-pedestrian-attributes branch from 7cac1f2 to 19bc76b Compare February 13, 2024 08:35
@pmai pmai merged commit 2466fcb into master Feb 13, 2024
6 checks passed
@pmai pmai deleted the 747-add-orientation-rate-to-bone-in-pedestrian-attributes branch February 13, 2024 09:03
@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 Apr 4, 2024
@pmai pmai added this to the V3.7.0 milestone Apr 4, 2024
@jdsika
Copy link
Contributor

jdsika commented Apr 23, 2024

reviewed for v3.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Add orientation rate to bone in pedestrian attributes
5 participants