Compute sun and moon positions #677
Conversation
…nverting to Cartesian
adjusted time from TAI to TDB
There was a problem hiding this comment.
You can just skip this, because rotating a vector by identity results in the same vector.
There was a problem hiding this comment.
Better to include the full exception message from Components: "The true anomaly of the hyperbolic orbit lies outside of the bounds of the hyperbola." Yeah, I don't really know what that means, either. Also it shouldn't ever happen for the Simon model. But there's no reason not to throw the better error message if you're going to check for it.
There was a problem hiding this comment.
I hate to say it, but there's an allocation here that needs to be changed to use a result parameter.
cleaned up more documentation
There was a problem hiding this comment.
This is fine. Kepler's equation is going to converge for the reasonable orbits of the Earth-moon barycenter and the Moon. At least it better! Since we'll probably never do it, let's change this TODO to just a regular explanation comment: "STK Components uses a numerical method to find the eccentric anomaly in the case that Kepler's equation does not converge. We don't expect that to ever be necessary for the reasonable orbits used here." We try to avoid TODOs in master (though there are some), instead preferring GitHub issues for thing we might actually do, and comments of explanation for things we probably won't.
There was a problem hiding this comment.
"Earth-centered inertial frame" here, too.
|
@kring anything else? Your diff comments above got off by a few lines, but I think I caught everything you mentioned. |
Source/Core/Cartesian3.js
Outdated
There was a problem hiding this comment.
Need to remove this, too, since it called the other one.
|
Almost done, just those couple of things left. Thanks for your patience with my endless comments. |
|
@kring I appreciate your thoroughness. Anything else? |
|
Tests pass, Sandcastle examples look good (especially the terrain one where you can see the sun and moon specular highlight), jsHint is happy. Code looks allocation-free! Just one last problem, I promise! I just noticed that |
|
@hpinkos just an FYI; we don't get automatic notification to every change, so when you are done with review comments, it's usually a good idea to write a quick "ready for another review" message. Otherwise @kring wouldn't know you finished the rename until he remembers to check. That being said, I'll let him merge this, since he's been the one reviewing it. |
|
Looks good. Thanks, @hpinkos! Merging. |
4 participants
Resolves: #391 #337
Ported parts of Simon1994PlanetaryPositions, KeplarianElements and ModifiedKeplarianElements from components to compute the sun and moon positions. Updated UniformState to use the new sun and moon positions.