Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Student #205
base: main
Are you sure you want to change the base?
Student #205
Changes from 149 commits
0610412
fa7c867
01e9f74
0242126
86c55ca
e967cc4
53a8a34
3993c6a
07047e4
935ccc4
dfa12e5
7cd2e31
e2b5351
ce85d15
eb4ba79
da4ba34
3b214e4
70c9a61
8ebaeea
0a0cdad
3cea066
608ef32
dd38d0c
62e97f5
b4080b3
8a67834
385ccb0
e28964e
438eca6
4c0457b
7f7f893
1494585
a18cd6a
195c28e
967b177
376a565
0c90588
78594d0
fc3a857
0a537a7
3d4b351
7f6f144
6f7c781
816f155
4b1e5d3
aba8df1
0b444dd
4a5ebee
270d859
67864ff
4a130b3
d8b6abe
a68f04d
9e4b78d
724e9f5
6ce55d7
3633631
5fe2dc3
ff93e5f
db97bd7
0f68861
0259866
c792502
e8ceb9e
ba0f57c
0d585f9
59c3599
a2ad9fb
64b1ece
28c55de
b645b3b
612b3b2
d818d3f
71c3f50
7dd542b
87e09b8
b6602e1
aa21573
55b6443
50ceeac
428a75e
acb6aee
9732308
7b6db6d
11eb784
2f9969f
8b1d525
ccd3712
aacaa42
93f6332
3ce7dae
b42eac9
0a66921
f0522e4
b56c076
ce74572
ddf1006
536f606
d31310f
bdcd11b
f64cc0e
dcdc6f1
30d108f
e95504b
5a3dbc2
ac935cc
437d908
437b8d9
ef25a18
7914692
21d9634
747a270
8e37a3b
4240289
dee082b
43b29d9
f257760
e94520a
e51829e
bb1056f
3822fb5
a2cb141
884c0a4
70a4e27
084ae6d
3f13190
a9dc4ab
695f40e
50f0456
f1c3037
ed4828f
4d7640b
5987af2
96e2564
0b2d549
df16bc4
e042180
e1fb6ac
3e117bd
333196d
07d215b
a8032b1
0dd8c56
ad72734
f0e1a15
ef1d0d7
a6ef070
ff192ef
bc5bfd1
2e77458
624966d
1d7df19
5d184cf
363c11f
e9d8fce
020e53b
54a8621
9245ccb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
isn't attitude and pointing vec the same?
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 problem is that a pointing vector alone is not sufficient to define the attitude (i.e., if you have a satellite pointing to a specific direction, all the rotations around the axis passing through the pointing vector will have a different attitude but the same pointing vector). So, the pointing vector definition is arbitrary, while the attitude is not.
Of course, in a practical application, if you have a preferred direction of appointing (e.g., Nadir if you want the camera to point to the ground). So, you can fix the attitude through (yaw, pitch, roll) angles and associate a target pointing vector to track.
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.
Maybe it would be good add a link to explanation of attitude? ideally in the readme with the minimal example?
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.
Yes, it is in my TODO list. But I am waiting that we are both happy with the class architecture, methods, and so on, to avoid to have to redo it.
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.
Now, if I actually want to use this as a user I most likely don't want my spacecraft to just tumble out of control. I think we should add a function to set the actors attitude? (allowing users to simulate corrective maneuvers)
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.
Good point. There are two solutions:
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 not just a function to setting the current pointing/attitude? Super simple to implement and allows you implement more complex interaction via custom parameters etc.
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 will add it. But, just to clarify: if you do not call
actor.set_attitude_disturbances(...)
, the attitude will stay fixed, because no disturbance will take place. Still, I agree with you that a setter will be useful.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 don't resolve if I should read because it will be hidden :P)
I would suggest to move the attitude into actor (maybe even base_actor?) since pointing / attitude of the actor can be interesting even if it is not disturbed or maybe even for ground station (think ground station antenna pointing)?
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.
Sorry, I am resolving stuff because there are so many comments that I lose track of what I have to implement xD
Well, it could be smart, but for the way it works, the attitude model works for satellites only. So, at some point we will need to implement a ground_station attitude 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.
Duplicate code of
spacecraft_body_model.py
, should probably be removed?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 removed it from the spacecraft_body_model, so you can access this from spacecraft actor.