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 errors thrown when picking terrain #3900

Merged
merged 10 commits into from
May 11, 2016
Merged

Fix errors thrown when picking terrain #3900

merged 10 commits into from
May 11, 2016

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented May 6, 2016

Fixes #3779.

The GlobeSurfaceTileProvider keeps a list of commands submitted for rendering. Let one of the commands contain a vertex array created from upsampling. After executing the draw calls but before rendering finishes, the tiles are processed (loading/creating/transforming imagery/terrain). During the processing, its possible that an upsampled terrain tile can receive its actual data. The upsampled vertex array will be destroyed and replaced with the new mesh. If a pick happens right after a render, the list of commands now has a destroyed vertex array from the upsampled mesh.

There was a couple of fixes that I tried, but they all failed because some places in the code expect the commands executed the previous frame to be able to execute again. This was just the first place it would fail. The solution was to reference count the vertex arrays.

TLDR; We should reference count all WebGL resources.

@mramato
Copy link
Contributor

mramato commented May 9, 2016

TLDR; We should reference count all WebGL resources.

👍 Totally agree with this (and the destroy patter we currently use as a whole should be replaced with reference counting.)

@pjcozzi
Copy link
Contributor

pjcozzi commented May 10, 2016

This fix seems awkward. Maybe it is the only approach, but can you walk me through it today along with the others that you tried.

@bagnell
Copy link
Contributor Author

bagnell commented May 10, 2016

@pjcozzi This is ready for another look.

After our dscussion offline, I went back and changed it so that vertex arrays are queued to be destroyed at the start of the next frame. It only happens for tiles that are updated with new data. Vertex arrays of tiles that are being unloaded still happen immediately. It wasn't as bad as I thought.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 10, 2016

This is a bit cleaner and more direct.

Update CHANGES.md.

Is it possible to add a reasonable test? Or is it not worth it?

@bagnell
Copy link
Contributor Author

bagnell commented May 11, 2016

@pjcozzi This is ready. I don't think I could reproduce it in a unit test. The only way I could reproduce the problem was to pick on mouse move and constant camera movement.

@pjcozzi pjcozzi merged commit f7abdd4 into master May 11, 2016
@pjcozzi pjcozzi deleted the terrain-picking-fix branch May 11, 2016 21:54
mramato added a commit that referenced this pull request May 11, 2016
As part of #3900, `vertexArraysToDestroy` was added as a parameter to
`GlobeSurfaceTile.processStateMachine`, but none of the tests actually
pass that parameter.  This happens to be OK in master because none of
the tests end up using the parameter, but in the 3D Tiles branch, the
parameter is required and merging in master results in failing tests.

Passing in an empty array fixes the issue (since we don't care about
destroying the data in the specs), but we might possibly want to actually
test that the parameter is being used correctly.
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.

throwOnDestroy errors when terrain and picking are enabled
3 participants