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 #764

Merged
merged 40 commits into from
May 15, 2013
Merged

Sun #764

merged 40 commits into from
May 15, 2013

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented May 10, 2013

Adds sun visualization in 3D and Columbus view.

bagnell added 28 commits April 30, 2013 19:14
Conflicts:
	Source/Scene/SceneTransitioner.js
* // GLSL declaration
* const float czm_solarRadius = ...;
*/
// use toExponential instead of toString to prevent a number like 1.2e2 from expanding to 120
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do this for all GLSL built-ins as a best practice? E.g., to avoid copy/paste errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we care about the textual length of these numbers? toExponential will often be larger than toString, e.g. for Math.PI. Also, wouldn't 1.2e2 still be considered an int? Don't you want to instead use like a F or D suffix on the literals like in most languages? I don't know GLSL.

Copy link
Contributor

Choose a reason for hiding this comment

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

So apparently in C++/C#, only floating-point/real literal syntax allows exponential notation, even if the number would be integral, and even if the number doesn't have a decimal, e.g. 1e0 is a double in C#. Well, now I know.

};
}

scene._fullScreenCommand.renderState = context.createRenderState();
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though createRenderState does a cache lookup. It is still expensive since it needs to generate the key and search. These calls to createRenderState actually show up on the profiler:

image

Render states are not really meant to be dynamic like what we are doing with the scissor test below. However, that is a common use case that is going to become more important as we do more post-processing/deferred stuff. I think we can treat the scissor test as a special case so we can keep the render state optimizations we already have and still serve this case. Let's discuss.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 11, 2013

The profiler isn't too scary for this. Do you have before/after performance numbers on a desktop? Have you tried this on mobile? Should it be off by default?

command = entireFrustumCommands[commandName];
if (typeof command !== 'undefined') {
boundingVolume = command.boundingVolume;
if (typeof boundingVolume !== 'undefined' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though we know no one is using it yet here, we should still probably check command.cull to save us pain later.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 11, 2013

There are issues interacting with picking. Run the Sandcastle picking example and select "Layer Primitives", and the sun will flicker on mouse over, which executes a pick.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 11, 2013

JSHint is happy. The (somehwat incomplete) tests pass. Once we handle the above issues, I'll take a second pass over this.

@bagnell
Copy link
Contributor Author

bagnell commented May 13, 2013

@pjcozzi This is ready for another review pending the changes you want to make for the sun, stars and atmosphere commands. For scissor test changes, see 259ad7c.

I'll get some before/after performance numbers and test it on mobile.

@bagnell
Copy link
Contributor Author

bagnell commented May 13, 2013

The version in master is running at 181 fps. This version with the sun visible is running at 176 fps.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 14, 2013

The version in master is running at 181 fps. This version with the sun visible is running at 176 fps.

Great. That is basically free - as least under the GPU load for the default globe.

Conflicts:
	CHANGES.md
@bagnell
Copy link
Contributor Author

bagnell commented May 14, 2013

On the Asus tablet, the version in master runs at 13 fps and this version with the sun visible is 8 fps.

@mramato
Copy link
Contributor

mramato commented May 14, 2013

Just keep in mind that the Asus tablet we have is particularly slow and several years old. I haven't checked stats on my phone, but I can check out the sun branch tonight if you care.

@bagnell
Copy link
Contributor Author

bagnell commented May 14, 2013

@pjcozzi This is ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 15, 2013

Wonderful. Merging.

pjcozzi added a commit that referenced this pull request May 15, 2013
@pjcozzi pjcozzi merged commit b2d01b1 into master May 15, 2013
@pjcozzi pjcozzi deleted the sun branch May 15, 2013 13:41
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