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

Automatic Globe depth texture #2506

Merged
merged 179 commits into from
May 28, 2015
Merged

Automatic Globe depth texture #2506

merged 179 commits into from
May 28, 2015

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Feb 21, 2015

Creates an automatic uniform for the depth texture after the globe is rendered.

@@ -203,6 +205,19 @@ define([

/**
* @memberof UniformState.prototype
* @type {Texture}
*/
globeDepthTexture : {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just replace this with a regular property. It will be slightly faster and more concise.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 21, 2015

@chris-cooper just a heads up that this branch may cause some merge conflicts for the postprocess-hook branch.

function destroyTextures(scene) {
scene._colorTexture = scene._colorTexture && !scene._colorTexture.isDestroyed() && scene._colorTexture.destroy();
scene._depthStencilTexture = scene._depthStencilTexture && !scene._depthStencilTexture.isDestroyed() && scene._depthStencilTexture.destroy();
scene._depthStencilCopyTexture = scene._depthStencilCopyTexture && !scene._depthStencilCopyTexture.isDestroyed() && scene._depthStencilCopyTexture.destroy();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not declared in the constructor function. Why not name it _depthStencilGlobeTest? "Copy" is too generic, especially as we move towards a post-processing framework.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 21, 2015

I get culling/blending issues in the Geometry and Appearances Sandcastle example (but not on cesiumjs.org). For example, zoom into the corridors near Alaska and they disappear and reappear.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 21, 2015

Exception on resize:

image

'void main() { gl_FragColor = vec4(texture2D(depthTexture, v_textureCoordinates).r); }\n';
scene._copyDepthCommand = context.createViewportQuadCommand(copyDepthFS, {
renderState : context.createRenderState(),
uniformMap : {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just remove this and use the default, right?

@bagnell
Copy link
Contributor Author

bagnell commented Mar 5, 2015

I get culling/blending issues in the Geometry and Appearances Sandcastle example

I couldn't reproduce this. Is it the blinking that I showed you?

I figured out that the blinking is caused when the _copyDepthCommand command is executed, but I'm not sure why. Any ideas?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 5, 2015

On Windows, this happens in Chrome, but not IE and Firefox. All of which support floating-point textures, but only Chrome supports multiple render targets, so perhaps this is an OIT issue when MRT is available. (This didn't happen on Chrome/Mac, which I bet doesn't have MRT, but I have to check).

If I try to disable OIT in the Geometry and Appearances example,

var viewer = new Cesium.Viewer('cesiumContainer', { 
    orderIndependentTranslucency : false,
    infoBox : false });

we crash:

TypeError: Cannot read property 'setColorFramebuffer' of undefined
TypeError: Cannot read property 'setColorFramebuffer' of undefined
at updateFramebuffers (http://localhost:8081/Source/Scene/Scene.js:905:23)
at new Scene (http://localhost:8081/Source/Scene/Scene.js:463:9)
at new CesiumWidget (http://localhost:8081/Source/Widgets/CesiumWidget/CesiumWidget.js:235:25)
at new Viewer (http://localhost:8081/Source/Widgets/Viewer/Viewer.js:342:28)
at startup (:6:14)
at Object.context.execCb (http://localhost:8081/ThirdParty/requirejs-2.1.9/require.js:1636:33)
at Object.Module.check (http://localhost:8081/ThirdParty/requirejs-2.1.9/require.js:870:51)
at Object. (http://localhost:8081/ThirdParty/requirejs-2.1.9/require.js:1111:34)
at http://localhost:8081/ThirdParty/requirejs-2.1.9/require.js:129:23
at http://localhost:8081/ThirdParty/requirejs-2.1.9/require.js:1154:21
Uncaught TypeError: Cannot read property 'setColorFramebuffer' of undefined (on line 325 of http://localhost:8081/Source/Widgets/CesiumWidget/CesiumWidget.js)

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 5, 2015

Have you stepped through the code? Do you know if createFramebuffers is getting called during zoom when the artifact occurs.

Are we somehow reading and writing to the same texture. Have we tried different texture formats? How can we narrow it down?

I couldn't reproduce this. Is it the blinking that I showed you?

Probably, but I couldn't reproduce the blinking in the same way you had it.

@mramato
Copy link
Contributor

mramato commented Mar 5, 2015

As discussed offline, I think this branch accidentally fixes #2327 as well. I have no idea why.

While I'm not qualified to review this code, I did notice harsher edges in this branch compared to master. For example, the box geometry demo:

master
image

this branch.
image

@pjcozzi
Copy link
Contributor

pjcozzi commented May 27, 2015

With the "add billboard" button, the label shows up in the terrain:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented May 27, 2015

Ah, is this because the peak is so steep and it is getting a lower height next to it? Unless you have a simple solution, make a note of this in #2685.

@@ -144,6 +144,50 @@
}, Cesium.ScreenSpaceEventType.MOUSE_MOVE);
});

Sandcastle.addToolbarButton('Pick position', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why we get negative altitudes right at the end of the model?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, its picking the depth plane. The regular pick to see if the model is under the mouse cursor checks a 3x3 pixel region. Picking the position only uses the center pixel.

* Added new `PointPrimitive` and `PointPrimitiveCollection`, which are faster and use less memory than billboards with circles.
* Changed `Entity.point` back-end graphics to use the new `PointPrimitive` instead of billboards. No change to the `Entity.point` API.
* Added optional drilling limit to `Scene.drillPick`.
* Added optional `ellipsoid` parameter to construction options of imagery and terrain providers that were lacking it. Note that terrain bounding spheres are precomputed on the server, so any supplied terrain ellipsoid must match the one used by the server.
* Upgraded Autolinker from version 0.15.2 to 0.17.1.
* Fixed documentation for `Context.drawingBufferHeight` and `Context.drawingBufferWidth`.
* Added debug option to `Scene` to show the depth buffer information for a specified view frustum slice and exposed capability in `CesiumInspector` widget.
* Removed `Scene.fxaaOrderIndependentTranslucency`. Use `Scene.fxaa` which is now `true` by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we talk about this already?

Shouldn't we deprecate this (over one release is fine) so we output the console warning in the setter?

@bagnell
Copy link
Contributor Author

bagnell commented May 27, 2015

Ah, is this because the peak is so steep and it is getting a lower height next to it?

Right the position of that billboard is actually slightly yo the left of the peak. I'll make a note of it.

*/
this.fxaaOrderIndependentTranslucency = true;
this.debugShowDepthFrustum = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be debugShowGlobeDepthFrustum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its also used for showing the pick depth buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK.

* @default false
* @private
*/
this.copyGlobeDepth = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should I test it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we set it to true and then test for the automatic uniform?

@pjcozzi
Copy link
Contributor

pjcozzi commented May 27, 2015

This works well enough. Just those minor comments then let's get this merged.

@mramato we should probably do the entity API changes post 1.10.

@bagnell
Copy link
Contributor Author

bagnell commented May 27, 2015

@pjcozzi This is ready for another look.

pjcozzi added a commit that referenced this pull request May 28, 2015
@pjcozzi pjcozzi merged commit 7ef77c8 into master May 28, 2015
@pjcozzi pjcozzi deleted the globe-depth branch May 28, 2015 20:40
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.

5 participants