Conversation
GabrieleMeoni
left a comment
GabrieleMeoni
left a comment
There was a problem hiding this comment.
I think this is fine.
gomezzz
left a comment
gomezzz
left a comment
There was a problem hiding this comment.
@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.
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.
"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.
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.
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.
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.
the structure should match existing code here
PASEOS/paseos/actors/spacecraft_actor.py
Line 22 in c7a9030
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.
(some of those functions should also be added)
There was a problem hiding this comment.
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.
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.
why is this a property and the other one not?
|
( @Moanalengkeek pls disregard, moving into discussion with Gabriele about paseos integration) |
3 participants
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