-
Notifications
You must be signed in to change notification settings - Fork 9
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
Creating Geometric Model #195
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.
I think this is fine.
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.
@GabrieleMeoni I put some comments here to elaborate, also note there are 0 tests in this PR
Args: | ||
actor (SpacecraftActor): Actor to update. | ||
mass (float): Mass of the spacecraft in kg. | ||
vertices (list): List of all vertices of the mesh in terms of distance (in m) from origin of body 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.
no usually they are 3d points not just distances
actor: SpacecraftActor, mass: float, vertices=None, faces=None, scale: float = 1 | ||
): | ||
"""Define geometry of the spacecraft actor. This is done in the spacecraft body reference frame, and can be | ||
transformed to the inertial/PASEOS reference frame using the reference frane transformations in the attitude |
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.
"frane" -> frame (I can't suggest changes because already merged)
triangles. Uses Trimesh to create the mesh from this and the list of vertices. | ||
scale (float): Parameter to scale the cuboid by, defaults to 1. | ||
""" | ||
assert mass > 0, "Mass is > 0" |
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.
should check that shape of vertices and faces is N,3, and that all entries in faces are < len(vertices)
""" | ||
assert mass > 0, "Mass is > 0" | ||
|
||
actor._mass = mass |
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.
should check if mass is set already and give a warning because it might be overwritten
geometric_model = GeometricModel( | ||
local_actor=actor, actor_mass=mass, vertices=vertices, faces=faces, scale=scale | ||
) | ||
actor._mesh = geometric_model.set_mesh() |
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.
a set_ function usually gets something passed and sets it in an object but see comment on structure in geometric model
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 structure should match existing code here
PASEOS/paseos/actors/spacecraft_actor.py
Line 22 in c7a9030
_thermal_model = None |
the actor has the models as an attribute. the reason is that (what is completely missing in this PR) the users needs to be able to get info from the actor
actor.get_rotation
actor.get_mesh
actor.get_whatever
to match existing code this functions should be in the actor
this doesn't work if the actor is only a parameter of the model
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 of those functions should also be added)
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.
would rename to spacecraft_body_model
and put it in the actor folder. "geometric" is very vague and it could also be the geometry of the central body
self._actor_moment_of_inertia = self._actor_mesh.moment_inertia | ||
return self._actor_moment_of_inertia |
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.
this function doesn't do anything right now, so please raise a NotImplemented error so it's clear that nothing happens
self._actor_mesh = mesh.apply_scale(self.scale) | ||
return self._actor_mesh | ||
|
||
@property |
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.
why is this a property and the other one not?
( @Moanalengkeek pls disregard, moving into discussion with Gabriele about paseos integration) |
Description
Summary of changes
Replaces pull request #192 as that one had problems with creating a pull request from a fork.
See #192 for further information