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

Gl context loss #1234

Merged
merged 2 commits into from
Jan 26, 2017
Merged

Gl context loss #1234

merged 2 commits into from
Jan 26, 2017

Conversation

karimnaaji
Copy link
Member

@karimnaaji karimnaaji commented Jan 17, 2017

Revisit a little bit our context loss on Android:

  • Remove index generation that was being used to track whenever a new context is created
  • gl objects no longer retain the memory
  • Reload entirely the scene onSurfaceCreated whenever the map view has not been destroyed

Fixes #1206

Copy link
Member

@matteblair matteblair left a comment

Choose a reason for hiding this comment

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

I really do enjoy seeing all of that 'generation tracking' disappear.

However right now these changes result in a noticeable delay in resuming an app from pause. It appears that after resuming, all of the tiles in view are rebuilt over a few seconds, whereas previously there was no noticeable delay in resuming from pause. I'm not totally sure what's causing this yet, but we should nail it down and resolve it before merging - we don't want to introduce a regression in user experience.

Update: I am seeing the same delay now on master as well so it doesn't appear that this change introduces that effect. This effect is notably absent in eraser-map (the map resumes instantaneously), so I want to know what changed that caused this. However that concern isn't blocking this PR.

@@ -811,6 +812,8 @@ public void queueSceneUpdate(SceneUpdate sceneUpdate) {
* @param sceneUpdates List of {@code SceneUpdate}
*/
public void queueSceneUpdate(List<SceneUpdate> sceneUpdates) {
this.sceneUpdates.addAll(sceneUpdates);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also add updates from the method above this one to the list.

@@ -1073,7 +1078,13 @@ public void onSurfaceChanged(GL10 gl, int width, int height) {

@Override
public void onSurfaceCreated(GL10 gl, EGLConfig config) {
if (surfaceCreated) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow the logic of this. Can you elaborate or add comments?

@matteblair matteblair force-pushed the gl-context-loss branch 2 times, most recently from 5b770c9 to a1af3e7 Compare January 24, 2017 23:06
Copy link
Member

@matteblair matteblair left a comment

Choose a reason for hiding this comment

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

This seems to work well now!

@karimnaaji I made some changes, effectively reverting one of your commits. See if this still makes sense to you.

@karimnaaji
Copy link
Member Author

Ok I will do some testings tomorrow and see how this goes!

@karimnaaji karimnaaji force-pushed the gl-context-loss branch 4 times, most recently from dfadc8d to f21ead4 Compare January 25, 2017 22:40
@karimnaaji karimnaaji merged commit c35ba6a into master Jan 26, 2017
@karimnaaji karimnaaji deleted the gl-context-loss branch January 26, 2017 14:29
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.

2 participants