Added compute pass #2993

lilleyse · 2015-09-01T22:49:28Z

Still a work in progress.

Some thoughts:

I decided to allow the ComputeCommand to accept a custom vao instead of supporting subdivisions. Having a custom vao, especially the one in ImageryLayer, seems like a special case, and it might be annoying to generalize that.
I pass in a Texture object instead of creating it fresh inside ComputeEngine. The object that called the compute command will need that texture back anyway.
I create and destroy FBO's for every compute command, I think there should be a more efficient solution.
Sun and ImageryLayers are both special cases, and don't act like primitives. So the code that actually handlesComputeCommand in Scene isn't doing anything right now.
ComputeEngine is inside Context but maybe should go in Scene instead. I did this for now because ImageryLayer only has a reference to the context.

pjcozzi · 2015-09-01T23:21:05Z

Source/Renderer/ComputeCommand.js

+        this.outputTexture = options.outputTexture;
+
+        /**
+         * Whether this command will persist beyond this call.


It's not the command that would persist; it is the renderer resources, i.e., framebuffer, etc.

pjcozzi · 2015-09-01T23:37:52Z

@lilleyse good start!

I decided to allow the ComputeCommand to accept a custom vao instead of supporting subdivisions. Having a custom vao, especially the one in ImageryLayer, seems like a special case, and it might be annoying to generalize that.

OK, I don't see any other uses cases as of now so it could be dangerous to attempt to reasonably abstract this.

I pass in a Texture object instead of creating it fresh inside ComputeEngine. The object that called the compute command will need that texture back anyway.

Agreed.

I create and destroy FBO's for every compute command, I think there should be a more efficient solution.

Yes, we can cache it as we previously discussed. We can brainstorm tomorrow if you want.

Sun and ImageryLayers are both special cases, and don't act like primitives. So the code that actually handles ComputeCommand in Scene isn't doing anything right now.

It will; see my comments. We will need special case handling for the sun.

ComputeEngine is inside Context but maybe should go in Scene instead.

Agreed.

lilleyse · 2015-09-02T21:35:00Z

@pjcozzi

I found the cause of the ImageryLayer problem. All the reproject commands share the same VAO, but stream new data into it's second vertex buffer. This is fine when the compute commands are executed synchronously, but when the commands are issued and processed later, all of them use the vertex data from the last reproject call.

The fix would be to generate a new vao and buffer for every reproject call. There could be a pooling system to reuse buffers, but overall it seems like a bad idea.

The good thing is the reprojection code always happens during the render loop, and not as a reaction to the image load. So with all that in mind, I think the reproject code should stay as it is, and not use the command list. What do you think?

pjcozzi · 2015-09-02T21:54:48Z

All the reproject commands share the same VAO, but stream new data into it's second vertex buffer.

Ah, I should have thought of this.

So with all that in mind, I think the reproject code should stay as it is, and not use the command list. What do you think?

You mean the imagery reprojection shouldn't use the new ComputeCommand? I still think it should since we only want the Scene execute commands, not primitives.

The fix would be to generate a new vao and buffer for every reproject call. There could be a pooling system to reuse buffers, but overall it seems like a bad idea.

I think it is fine for Globe (or something it owns) to contain a buffer with the positions, then each compute command can create a new buffer (now STATIC instead of STREAM) with the other attribute and a VAO...or a ComputeCommand could have a callback to get resources...just beware of the architect astronaut as I warned.

lilleyse · 2015-09-02T23:04:00Z

I'm worried that creating a buffer for every compute command would slow everything down. Though maybe it's not much worse than streaming. The resource callback works, but seems hacky...

pjcozzi · 2015-09-03T00:55:13Z

The callback to provide renderer resources should be pretty clean and just as efficient as what we are doing today.

Creating and destroying buffers could be worse, but we don't know what the driver is doing...it is probably pooling them...this is WebGL, not Vulkan...we don't have explicit memory management, just educated guesses. The benefit is we would be using STATIC, not STREAM and the bus traffic is there in either case, but worse in the case where the data is interleaved. In this specific case, it probably won't matter either way, but let's explore the callback route.

lilleyse · 2015-09-03T18:31:41Z

Updated with the suggested changes.

pjcozzi · 2015-09-04T12:20:07Z

CHANGES.md

@@ -6,8 +6,8 @@ Change Log
 * Breaking changes
  * Remove deprecated `AxisAlignedBoundingBox.intersect` and `BoundingSphere.intersect`. Use `BoundingSphere.intersectPlane` instead.
  * Remove deprecated `getFeatureInfoAsGeoJson` and `getFeatureInfoAsXml` constructor parameters from `WebMapServiceImageryProvider`.
-* Added support for `GroundPrimitive` which works much like `Primitive` but it drapes the geometry over terrain. Valid geometries that can be draped on terrain are `CircleGeometry`, `CorridorGeometry`, `EllipseGeometry`, `PolygonGeometry`, and `RectangleGeometry`.  Because of the cutting edge nature of this feature in WebGL, it requires the [EXT_frag_depth](https://www.khronos.org/registry/webgl/extensions/EXT_frag_depth/) extension, which is currently only supported in Chrome, Firefox, and Edge.  Apple support is expected in iOS 9 and MacOS Safari 9. Android support varies by hardware and IE11 will most likely never support it. You can use [webglreport.com](http://webglreport.com) to verify support for your hardware  Finally, this feature is currently only supported in Primitives and can not be used via the Entity API yet.
-* Added `Scene.groundPrimitives`, which is a primitive collection like `Scene.primitives`, but for `GroundPrimitive`s. Use for correct z-ordering. For example:
+* Added support for `GroundPrimitive` which works much like `Primitive` but drapes geometry over terrain. Valid geometries that can be draped on terrain are `CircleGeometry`, `CorridorGeometry`, `EllipseGeometry`, `PolygonGeometry`, and `RectangleGeometry`.  Because of the cutting edge nature of this feature in WebGL, it requires the [EXT_frag_depth](https://www.khronos.org/registry/webgl/extensions/EXT_frag_depth/) extension, which is currently only supported in Chrome, Firefox, and Edge.  Apple support is expected in iOS 9 and MacOS Safari 9. Android support varies by hardware and IE11 will most likely never support it. You can use [webglreport.com](http://webglreport.com) to verify support for your hardware. Finally, this feature is currently only supported in Primitives and not yet available via the Entity API. [#2865](https://github.com/AnalyticalGraphicsInc/cesium/pull/2865)


Is there a branch issue? These updates to CHANGES.md should not be here.

My branch was out of date with master and I pulled. Usually the rebase works fine. I'm not sure what went wrong this time, maybe I pulled master before I pulled the compute-pass branch which had your commit. Unless it's easy to fix this, I should open a new pull request.

Could be easy, I'm the wrong person to ask. @mramato might know.

pjcozzi · 2015-09-04T12:43:15Z

Source/Scene/ScreenSpaceCameraController.js

@@ -465,7 +465,11 @@ define([
        var camera = scene.camera;
        var mode = scene.mode;

-        var pickedPosition = mode !== SceneMode.SCENE2D ? pickGlobe(object, startPosition, scratchPickCartesian) : camera.getPickRay(startPosition, scratchZoomPickRay).origin;
+        var pickedPosition;


Is this change from a merge issue?

Same comment for build.xml and package.json below. Perhaps your branch is just out of date?

pjcozzi · 2015-09-04T12:48:18Z

Looks good. What all is missing? Just unit tests for ComputeCommand? Recall, we discussed creating a mock ComputeCommand that sets one texel in a texture, and then verify the texel color in a second draw (perhaps use ViewportQuadPrimitive).

lilleyse · 2015-09-04T22:01:22Z

I added some specs and updates. There are still a few things to resolve.

pjcozzi · 2015-09-05T17:52:34Z

Let me know when you want me to look at this again.

lilleyse · 2015-09-05T22:08:38Z

It's ready for another check. I'm not sure if you looked over the ComputeCommand specs yet. I used a mock command primitive like I saw in another file. Let me know if I should do it differently.

I was able to clean up the commits using git rebase -i and a force push. Thankfully this is safe to do on this isolated branch.

pjcozzi · 2015-09-06T20:19:54Z

@lilleyse can you submit a new issue ("Renderer roadmap") with ideas you have for the compute pass but that we don't have the time to do now? Moving forward, we'll use this issue for the entire renderer roadmap. For an example, roadmap issue, see #927.

pjcozzi · 2015-09-06T20:28:05Z

Source/Renderer/ComputeEngine.js

+        var vertexArray = computeEngine._viewportQuadVertexArray;
+
+        if (!defined(vertexArray)) {
+            var geometry = new Geometry({


We have similar code here. Perhaps we should make a @private ViewportQuadGeometry in Core and use it in both places? For a simple-ish example geometry, see BoxGeometry.

pjcozzi · 2015-09-06T20:43:51Z

I'm not sure if you looked over the ComputeCommand specs yet.

Just looked at them. They look great.

pjcozzi · 2015-09-06T20:45:22Z

Just those minor comments then this is ready to merge.

In separate PRs, I would like us to look at:

Refactor/remove *Command.execute()
Pass just frameState to .update()
- Related: separate command lists to avoid compares like this.

lilleyse · 2015-09-08T13:36:13Z

Updated. ViewportQuadGeometry could be simplified, but I kept it with the style of the other Geometry files.

lilleyse · 2015-09-08T18:27:10Z

I replaced the previous commit with this. Keeping viewport quad creation in Context for now.

pjcozzi · 2015-09-08T21:40:28Z

Looks good!

Added compute pass

pjcozzi reviewed Sep 1, 2015
View reviewed changes

pjcozzi reviewed Sep 4, 2015
View reviewed changes

lilleyse and others added 5 commits September 5, 2015 16:41

Added compute pass

a0b0228

Update comment

cacfbee

Organized ComputeEngine and Sun compute command

7829617

Updated ImageryLayer and associated specs

f6a546b

Added ComputeCommand specs

fd12159

lilleyse force-pushed the compute-pass branch from 984861b to fd12159 Compare September 5, 2015 20:43

Sun compute pass only happens when sun is visible

9484bc0

lilleyse force-pushed the compute-pass branch from 32edae0 to 9484bc0 Compare September 5, 2015 22:02

pjcozzi reviewed Sep 6, 2015
View reviewed changes

Moved viewport quad creation

ddd259f

lilleyse force-pushed the compute-pass branch from 480294a to ddd259f Compare September 8, 2015 18:25

pjcozzi added a commit that referenced this pull request Sep 8, 2015

Merge pull request #2993 from lilleyse/compute-pass

a6bc070

Added compute pass

pjcozzi merged commit a6bc070 into CesiumGS:master Sep 8, 2015

pjcozzi deleted the compute-pass branch September 8, 2015 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added compute pass #2993

Added compute pass #2993

lilleyse commented Sep 1, 2015

pjcozzi Sep 1, 2015

pjcozzi commented Sep 1, 2015

lilleyse commented Sep 2, 2015

pjcozzi commented Sep 2, 2015

lilleyse commented Sep 2, 2015

pjcozzi commented Sep 3, 2015

lilleyse commented Sep 3, 2015

pjcozzi Sep 4, 2015

lilleyse Sep 4, 2015

pjcozzi Sep 5, 2015

pjcozzi Sep 4, 2015

pjcozzi Sep 4, 2015

pjcozzi commented Sep 4, 2015

lilleyse commented Sep 4, 2015

pjcozzi commented Sep 5, 2015

lilleyse commented Sep 5, 2015

pjcozzi commented Sep 6, 2015

pjcozzi Sep 6, 2015

pjcozzi commented Sep 6, 2015

pjcozzi commented Sep 6, 2015

lilleyse commented Sep 8, 2015

lilleyse commented Sep 8, 2015

pjcozzi commented Sep 8, 2015

Added compute pass #2993

Added compute pass #2993

Conversation

lilleyse commented Sep 1, 2015

pjcozzi Sep 1, 2015

Choose a reason for hiding this comment

pjcozzi commented Sep 1, 2015

lilleyse commented Sep 2, 2015

pjcozzi commented Sep 2, 2015

lilleyse commented Sep 2, 2015

pjcozzi commented Sep 3, 2015

lilleyse commented Sep 3, 2015

pjcozzi Sep 4, 2015

Choose a reason for hiding this comment

lilleyse Sep 4, 2015

Choose a reason for hiding this comment

pjcozzi Sep 5, 2015

Choose a reason for hiding this comment

pjcozzi Sep 4, 2015

Choose a reason for hiding this comment

pjcozzi Sep 4, 2015

Choose a reason for hiding this comment

pjcozzi commented Sep 4, 2015

lilleyse commented Sep 4, 2015

pjcozzi commented Sep 5, 2015

lilleyse commented Sep 5, 2015

pjcozzi commented Sep 6, 2015

pjcozzi Sep 6, 2015

Choose a reason for hiding this comment

pjcozzi commented Sep 6, 2015

pjcozzi commented Sep 6, 2015

lilleyse commented Sep 8, 2015

lilleyse commented Sep 8, 2015

pjcozzi commented Sep 8, 2015