Skip to content
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

Moon #1189

Merged
merged 28 commits into from
Oct 4, 2013
Merged

Moon #1189

merged 28 commits into from
Oct 4, 2013

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Sep 26, 2013

Adds the Earth's moon.

@mramato
Copy link
Contributor

mramato commented Sep 26, 2013

Nice. The moon seems to elongate as it gets to the edges of the view. Is this because of the fov? Or is something else going on. I'm not saying it's a bug, I just don't understand why it happens.

@@ -72,6 +72,7 @@ Beta Releases
becomes

return primitives.add(new Primitive(/* ... */));
* Added `Moon`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expand a bit. We also added Iau2000Orientation.ComputeMoon.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 26, 2013

Do you get this failure in Firefox?

Core/IauOrientationAxes compute ICRF to Moon Fixed. (show summary)
Expected { 0 : 0.784227052091917, 1 : -0.6200619152508556, 2 : -0.022608671404182445, 3 : 0.5578471124601639, 4 : 0.7205566654668133, 5 : -0.41183090094261243, 6 : 0.27165148607559436, 7 : 0.3103567513471994, 8 : 0.9109797785934294 } to equal { 0 : 0.784227052091917, 1 : -0.6200619152508556, 2 : -0.022608671404182448, 3 : 0.5578471124601639, 4 : 0.7205566654668133, 5 : -0.41183090094261243, 6 : 0.27165148607559436, 7 : 0.3103567513471994, 8 : 0.9109797785934294 }.

destroyObject,
Iau2000Orientation,
IauOrientationAxes,
JulianDate,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't use JulianDate. It also won't use Iau2000Orientation after some changes.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 26, 2013

@bagnell let's add showLabel properties to the sun and the moon (with the correct eye offsets) so they are easier to spot. It's probably best for each to have their own LabelCollection, and that it can return the Label so users can tweak the font, etc. Do this as a separate PR if you want.

I also encouraged @emackey to tweak the default FOV so we can see if we like it.

@emackey
Copy link
Contributor

emackey commented Sep 26, 2013

If you zoom way out from the Earth, and pan the view to put the Moon in the center of the screen and slowly zoom in, you won't see the Moon occlude the Earth even though it is in front of it. Can this be fixed?

moon_not_in_front_of_earth

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 26, 2013

Code looks good. Let's see if we can fix the z-order before merging.

@bagnell
Copy link
Contributor Author

bagnell commented Oct 2, 2013

@pjcozzi This is ready for another review.

@mramato
Copy link
Contributor

mramato commented Oct 3, 2013

Do we have a higher-res moon for the cesium-assets repository? I'd like to use this branch in an upcoming demo.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 3, 2013

*/
this.onlySunLighting = defaultValue(options.onlySunLighting, true);

this._ellipsoidPrimitive = new EllipsoidPrimitive();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want to merge #1194 and use the new ellipsoid options here?

@pjcozzi pjcozzi mentioned this pull request Oct 3, 2013
Conflicts:
	Source/Scene/EllipsoidPrimitive.js
@bagnell
Copy link
Contributor Author

bagnell commented Oct 3, 2013

How does this affect multi-frustum now that we need to include the moon?

With the current far-to-near ratio, the maximum number of frustums I've seen is 3. For horizon views with the moon visible, the number of frustums is 3 and it is the same if the moon is not shown.

How many more commands overlap frustums from different views?

For views centered around the Earth, it will only add one command for the last frustum. If the view is centered on the moon, then the number of commands is one or two where it can overlap the first two frustums.

Conflicts:
	CHANGES.md
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 4, 2013

Is this ready?

@bagnell
Copy link
Contributor Author

bagnell commented Oct 4, 2013

@pjcozzi This is ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 4, 2013

Do you get this test failure?

image

@bagnell
Copy link
Contributor Author

bagnell commented Oct 4, 2013

Yes, I forgot to update the tests. They're fixed now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 4, 2013

Looks good. We'll need do the labels at some point. #1218

pjcozzi added a commit that referenced this pull request Oct 4, 2013
@pjcozzi pjcozzi merged commit 4632b77 into master Oct 4, 2013
@pjcozzi pjcozzi deleted the moon branch October 4, 2013 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants