-
Notifications
You must be signed in to change notification settings - Fork 69
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
Conversation
|
||
InertiaTensor<RigidPart> const& part_inertia_tensor = | ||
part->inertia_tensor(); | ||
pile_up_inertia_tensor += part->inertia_tensor().Transform(part_to_pile_up); | ||
// NOTE(phl): The following call reads like the tensor "transforms" the | ||
// rigid motion. Improve this API. |
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.
The thing that would be consistent with the rest of our APIs would be an operator() on RigidTransformation I think; not sure how feasible that is...
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.
Agreed, but not today.
@@ -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(); |
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.
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.
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.
As discussed, this happens once when the pile-up is constructed/updated, so O(N).
ksp_plugin/pile_up.cpp
Outdated
|
||
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()); |
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.
part_to_barycentric.angular_velocity_of_to_frame
is the angular velocity of Barycentric
; that feels wrong somehow.
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.
Change to part_to_pile_up
which is more logical but not different.
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.
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.
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 by switching everything to RigidPileUp
.
Wedge(part_degrees_of_freedom.position() - RigidPileUp::origin, | ||
part->inertia_tensor().mass() * | ||
part_degrees_of_freedom.velocity()) * | ||
Radian; |
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.
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.
Not used much at this point. Tested (a bit) in game.