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

Fix texture loading and re-projection bug #3696

Merged
merged 8 commits into from
Mar 15, 2016
Merged

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Mar 10, 2016

Move loading terrain and imagery to the end of the quadtree update. Queue any texture re-projections to be executed the next frame.

For #3690.

…ueue any texture re-projections to be executed the next frame.
*/
ImageryLayerCollection.prototype.update = function(frameState) {
var layers = this._layers;
for (var i = 0, len = layers.length; i < len; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Put length outside loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind.

@lilleyse
Copy link
Contributor

I confirmed that this fixes the problem. The code looks fine, but maybe @pjcozzi or @mramato should merge.

@hpinkos
Copy link
Contributor

hpinkos commented Mar 10, 2016

@bagnell can you add tests for the new functions?

@bagnell
Copy link
Contributor Author

bagnell commented Mar 10, 2016

@hpinkos Not really. I added the one that I could. The code runs in the tests but not individually unit tested.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 10, 2016

The code is OK with me. @kring would be the best person to review if he has time in the next day or two.

// update collection: imagery indices, base layers, raise layer show/hide event
imageryLayers._update();
// update each layer for texture reprojection.
imageryLayers.update(frameState);
Copy link
Member

Choose a reason for hiding this comment

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

It's kind of dodgy to have two update methods on ImageryLayer. How about calling update queueReprojectionCommands cause that's what it actually does, right?

@kring
Copy link
Member

kring commented Mar 11, 2016

Looks good to me other than the one minor comment. Could you say a sentence or two about what was actually happening to cause the imagery to refresh over and over? It's not obvious to me how queuing the commands immediately was causing that.

@bagnell
Copy link
Contributor Author

bagnell commented Mar 11, 2016

This is ready.

@kring I'm not exactly sure. It wasn't queueing commands that was the issue. It was loading terrain and imagery at the beginning of the frame. I had a bunch of problems using processTileLoadQueue if it wasn't called immediately after enqueueing the render commands and before tileProvider.endUpdate. I changed the QuadtreePrimitive to have beginUpdate and endUpdate functions that are called once a frame, then, update can be called multiple times in between for multiple camera views. This change moves the terrain/imagery loading to the end of the frame. The other problem was that the imagery reprojection commands would be added at the end of the frame and would never be executed. That's why they are kept until the beginning of the next frame.

@kring
Copy link
Member

kring commented Mar 13, 2016

Ok, thanks for the explanation. I should have noticed this before, but this warrants a mention in CHANGES.md right?

@bagnell
Copy link
Contributor Author

bagnell commented Mar 14, 2016

@kring CHANGES.md was update. This is ready.

@mramato
Copy link
Contributor

mramato commented Mar 15, 2016

Since @kring approved everything and this is needed to fix the 3D Tiles branch, I'm going to merge this (with @bagnell's blessing).

mramato added a commit that referenced this pull request Mar 15, 2016
@mramato mramato merged commit ac78082 into master Mar 15, 2016
@mramato mramato deleted the imagery-reprojection branch March 15, 2016 18:47
@kring
Copy link
Member

kring commented Mar 15, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants