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

Sun Size #1046

Merged
merged 26 commits into from
Sep 10, 2013
Merged

Sun Size #1046

merged 26 commits into from
Sep 10, 2013

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Aug 20, 2013

Fixes #769. Doesn't turn the sun glow off but the size is correct.

@mramato
Copy link
Contributor

mramato commented Aug 21, 2013

I believe you if you say it's right now, but this looks ridiculously small.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 21, 2013

The code is fine. JSHint and tests pass.

I agree that this looks lame compared to before. STK makes the sun look much bigger by adding a large glow, and Google Earth has a huge sun.

There are several directions to improve this, e.g., do something similar to STK, scale up the sun by default with an option to change it, etc. Do what you think is best.

Also, when we redo the atmosphere (soonish), the sun may come for free for ground views, but that doesn't help us here.

@emackey
Copy link
Contributor

emackey commented Aug 22, 2013

I'm still experimenting with this, maybe I can get a better look for it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 26, 2013

@emackey any update?

@emackey
Copy link
Contributor

emackey commented Aug 26, 2013

Not yet, sorry, other projects calling me. Soon.

@emackey emackey mentioned this pull request Aug 26, 2013
@emackey
Copy link
Contributor

emackey commented Aug 27, 2013

I just showed Dan that the Sun's size with setGlowFactor(0) is still about 10% larger than seen in STK at the same FOV. He's looking into it.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 6, 2013

@emackey I changed the way the sun is sized again. You said you developed a few new ways to test it. Could you run through them. (The glow is temporarily broken).

@emackey
Copy link
Contributor

emackey commented Sep 6, 2013

The new sun size is an exact match to STK, perfect! For those following along, my testing methods are:

  1. In STK, use 3D window properties to set a large square 3D window (800 x 800, for example, square so that FOV is the same horizontally as vertically). In Globe Manager, turn off Sun->Sunshine, but make sure the Sun's globe is turned on. In advanced properties, set min visible distance to 1e9 km. Zoom out from the Earth, find the Sun, and crank down FOV to 1 degree so the Sun fills more than half the window. Take a screenshot of this into a paint program. Then set up the same scene at the same window size in Cesium, glowFactor zero, and paste that screenshot as a new layer, setting the opacity to about half. Then move the top layer until the Sun lines up with the bottom layer. It's pixel-perfect.

  2. Alternate method, find the distance from the Earth where the Sun has the same apparent size as the Earth. Less accurate, but not prone to FOV differences between rendering environments. STK shows the answer is roughly 1.38e6 km, and Cesium now agrees with this.

@emackey
Copy link
Contributor

emackey commented Sep 6, 2013

I think all of my review comments from @mramato are addressed now. I know @bagnell is still planning to fix the glow size. Also, master needs to merge in, then we're ready.

emackey and others added 3 commits September 6, 2013 10:47
Conflicts:
	Source/Shaders/Builtin/Constants.glsl
@bagnell
Copy link
Contributor Author

bagnell commented Sep 6, 2013

@emackey Can you check this once more? I fixed the glow and checked using your first method. With the glow its a little bigger, but that is expected because of the bloom filter.

@pjcozzi This is ready for a review.

@emackey
Copy link
Contributor

emackey commented Sep 6, 2013

I tweaked the strength of the "burst" and recalibrated the default to 1.0. Size is still correct. This is ready for master.

@emackey
Copy link
Contributor

emackey commented Sep 6, 2013

Just found an odd visual artifact. On my widescreen monitor, with Chrome maximized, the Sun is larger out near the edge. This is perhaps expected due to perspective, but in this case there's a specific "jump" in size as the sun moves in and out of the zone. Could be blooming turning on and off, maybe.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 6, 2013

This is ready. The size is right wherever it is on the screen and there is a property that can enable/disable bloom.

scene._sunBloom = scene.sunBloom;
} else if (!defined(scene.sun) && defined(scene._sunPostProcess)) {
scene._sunPostProcess = scene._sunPostProcess.destroy();
scene._sunBloom = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's cleaner to keep this as a boolean, i.e., false. I suspect we do that elsewhere.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 7, 2013

Tests and examples look good. This is ready pending my minor comments and an update to CHANGES.md.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 9, 2013

@pjcozzi This is ready for another review.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 10, 2013

Looks good.

pjcozzi added a commit that referenced this pull request Sep 10, 2013
@pjcozzi pjcozzi merged commit d74ba6a into master Sep 10, 2013
@pjcozzi pjcozzi deleted the sunSize branch September 10, 2013 12:07
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.

Ability to turn off sun glow...
4 participants