Add starfield layer #86
Conversation
extract ra, dec and vmag indexes from the metadata of the stars.json
moved stars.json to images folder reverted the longitude formula to "180 - normalizeAngle(GMST - ra)"
|
This isn't directly for the starfield, but now that we include an util for computing the position of the sun, we could let the atmosphere layer react to its time property as well. What do you think? |
|
Yann, I think that's a good idea, and the CelestialProjection class is a good start. I'm unsure of this class' name and place within the SDK. By name and some methods CelestialProjection seems related to GeographicProjection, but CelestialProjection is not a map projection and includes some utilities that appear more general. Would you be willing to review this class on the basis of its naming and its cohesion? |
|
Absolutely. I'll create an issue to track this idea of changing the Atmosphere layer a bit. We can first finalise the Starfield and this new util. Some reviews from my side are also pending here by the way. |
|
The goal of CelestialProjection currently boils down to computing the sun's position in geographic and celestial coordinates. celestialToGeographic, computeJulianDate and normalizeAngle are actually helpers to reach this purpose. I would thus remove them from the public interface and have computeSunCelestialLocation take a regular date as argument. This should help with the cohesion from the perspective of the public interface. I haven’t found such normalization of angles and conversions (celestialToGeographic and Julian date) elsewhere in the SDK so they could stay in this class in my opinion. Regarding the class name, I would have it relate to its core purpose. Maybe something like SunPosition with methods like getAsGeographicLocation and getAsCelestialLocation. Regarding the location of this class in the SDK, I don’t see a more appropriate place than the very generic util, at the moment. |
|
CelestialProjection was intended to be used for any celestial body. At the moment CelestialProjection is only useful for the Sun position, but it was originally implemented with the Moon as well and had the methods computeMoonCelestialLocation and computeMoonGeographicLocation. The methods celestialToGeographic, computeJulianDate and normalizeAngle are helpers that can be used for any celestial body. My reasoning was that if we want to add a new celestial body, it would only require a computeXCelestialLocation method for that particular object. All celestial bodies (stars, Sun, Moon) require converting to julianDate. There is a computeSunGeographicLocation method that takes a javascript Date are returns LatLon. Let me know if you want me to change anything. |
|
For me, it's fine. |
| ' float angleDivisions = angle / 360.0;\n' + | ||
| ' return 360.0 * (angleDivisions - floor(angleDivisions));\n' + | ||
| '}\n' + | ||
|
|
There was a problem hiding this comment.
@strikerM, could this be replaced by the built-in GLSL step function?
http://docs.gl/el3/step
src/util/SunPosition.js
Outdated
|
|
||
| return JD0h + dayFraction; | ||
| }, | ||
|
|
There was a problem hiding this comment.
@strikerM, This function looks appropriate for WWMath. Would you please move it there? Also, if you'd compare it to Android's WWMath.normalizeAngle we need to ensure that the two are equivalent.
removed normalizeAngle from SunPosition replaced if_gt function with the build in glsl step function
|
@pdavidc |
|
@strikerM with your latest commit, it seems you've removed the test associated with the normalizeAngle360 logic. Could you create a WWMath.test.js and add your test there instead? |
|
I've added the test. |
No description provided.