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

Propagate the dynamical elements from the parts to the pile-up #2397

Merged
merged 14 commits into from
Dec 14, 2019

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Dec 8, 2019

Not used much at this point. Tested (a bit) in game.

@@ -71,8 +67,7 @@ void Subset<Part>::Properties::Collect(
collected_ = true;
if (EqualsExistingPileUp()) {
PileUp& pile_up = *parts_.front()->containing_pile_up();
pile_up.set_mass(total_mass_);
pile_up.set_intrinsic_force(total_intrinsic_force_);
pile_up.RecomputeFromParts();
Copy link
Member

Choose a reason for hiding this comment

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

Aren't you making things quadratic in the number of parts instead of linear here? Part counts can get pretty big, and this runs at every frame; we should be careful.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed, this happens once when the pile-up is constructed/updated, so O(N).


Bivector<AngularMomentum, RigidPart> const part_angular_momentum =
Anticommutator(part_inertia_tensor,
part_rigid_motion.angular_velocity_of_to_frame());
part_to_barycentric.angular_velocity_of_to_frame());
Copy link
Member

Choose a reason for hiding this comment

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

part_to_barycentric.angular_velocity_of_to_frame is the angular velocity of Barycentric; that feels wrong somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Change to part_to_pile_up which is more logical but not different.

Copy link
Member

Choose a reason for hiding this comment

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

But still, this is the angular velocity of the pile up axes (i.e., of non rotating axes) in the axes of the part, rather than the angular velocity of the part as seen in non-rotating axes; I think that is a sign error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by switching everything to RigidPileUp.

// compute the overall force applied to it.
// TODO(phl): We assume that forces are applied at the centre of mass of the
// pile-up, but they are really applied at the centre of mass of the parts, so
// this introduces a torque.
Copy link
Member

Choose a reason for hiding this comment

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

They are not even applied at the centre of mass of the parts; they are applied at some position, which we do not currently transmit to the C++.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the comment.

Sorry, something went wrong.

Wedge(part_degrees_of_freedom.position() - RigidPileUp::origin,
part->inertia_tensor().mass() *
part_degrees_of_freedom.velocity()) *
Radian;
Copy link
Member

Choose a reason for hiding this comment

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

Please open an issue to track moving the computation of the momenta from the motion and the inertia to some place that can be unit-tested.

@eggrobin eggrobin added the LGTM label Dec 13, 2019
@pleroy pleroy merged commit fadba09 into mockingbirdnest:master Dec 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants