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

Multiple Globe updates #3593

Merged
merged 4 commits into from
Feb 18, 2016
Merged

Multiple Globe updates #3593

merged 4 commits into from
Feb 18, 2016

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Feb 17, 2016

The globe can be updated multiple times in a single frame to get terrain tiles for multiple render passes, for example, multiple camera views, viewports, framebuffers, etc.

scene.globe.initialize(frameState);

updateEnvironment(scene);

// set viewport/camera/ framebuffer

updatePrimitives(scene);
createPotentiallyVisibleSet(scene);
updateAndClearFramebuffers(scene, passState, color);
executeComputeCommands(scene);
executeCommands(scene, passState);

// update viewport/camera/framebuffer

updatePrimitives(scene);
createPotentiallyVisibleSet(scene);
executeCommands(scene, passState);

// any more updates/renders

resolveFramebuffers(scene, passState);
executeOverlayCommands(scene, passState);

scene.globe.finalize(frameState);

Globe.initialize and Globe.finalize (I'm open to name suggestions) will set up the globe for rendering and manage tile loading/unloading which only happens once per frame. Globe.update selects already loaded tiles for rendering based on the current view.

For #2594 and the infinite-2D branch.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 17, 2016

Nice, I'll look at this later today. @lilleyse can you review to see how this will work with your shadow changes?

@lilleyse
Copy link
Contributor

Yeah I'll check this out soon.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 17, 2016

I would like to get this into 3d-tiles soon (a branch off of 3d-tiles, then a pull request). What is the best approach? @bagnell do you want to do it? @lilleyse or would you rather do it to get more experience with this change?

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 17, 2016

In order to get this into 3D Tiles, we might need to add a separate primitive collection, e.g., scene.tilesets so that we can call initialize and finalize. Open to other ideas.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 17, 2016

As for naming, instead of initialize/finalize consider beginFrame/endFrame, the semantics better describe the lifetime (e.g. is initialize like a constructor function, and finalize like in Java?). I am tempted to name them something even more specific, but I don't know that we can really predict that much about their implementation.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 17, 2016

That's all my comments. @lilleyse can review and merge when ready.


if (width === 0 || height === 0) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not relevant to this PR, but is this a check we should be doing scene-wide?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also perhaps this should be a helper function since it comes up 3 times.

        if (!this.show) {
            return;
        }

        var context = frameState.context;
        var width = context.drawingBufferWidth;
        var height = context.drawingBufferHeight;

        if (width === 0 || height === 0) {
            return;
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

This seemed odd to me too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...but is this a check we should be doing scene-wide?

The code in Globe is some of the oldest in Cesium and this was probably a workaround that was never removed. I'm going to remove it since, you're right, this should be a scene-wide check.

@lilleyse
Copy link
Contributor

It might be helpful to add comments above initialize, update, and finalize eventually.

@lilleyse
Copy link
Contributor

There should be a test that checks the new functionality by calling update multiple times during a single frame.

@lilleyse
Copy link
Contributor

That's all from me.

@bagnell
Copy link
Contributor Author

bagnell commented Feb 18, 2016

@lilleyse I can't think of a good test that would fail in master and pass here. I'm going to wait for the infinitely scrolling 2D for a good test. I can place the camera near the edge of the map in 2D and in master the pixel would be the background color but with multiple updates it would be the color of the imagery.

I have updated everything else from the comments.

@lilleyse
Copy link
Contributor

Ok sounds good to me.

lilleyse added a commit that referenced this pull request Feb 18, 2016
@lilleyse lilleyse merged commit d3dcfb0 into master Feb 18, 2016
@lilleyse lilleyse deleted the globe-update branch February 18, 2016 18:16
@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 18, 2016

What's the plan for merging this into 3d-tiles?

@lilleyse
Copy link
Contributor

Just to make sure, does merging into 3d-tiles mean providing similar functionality for Cesium3DTileset?

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 18, 2016

Yes, I think we'll want to do beginFrame/endFrame for 3D Tiles sooner rather than later so they work with shadows and we have a better framework for cache replacement.

@lilleyse
Copy link
Contributor

I can work on bringing this into 3d-tiles.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 18, 2016

👍 Thanks.

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.

3 participants