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 ClientDataSource losing features after scene change #2118

Merged
merged 5 commits into from
Jan 3, 2020

Conversation

matteblair
Copy link
Member

@matteblair matteblair commented Dec 15, 2019

When the current scene is changed, the TileManager instance used by the old scene runs cancelTileTasks() to stop pending asynchronous work. The implementation of this method also called clearData() on all of the managed TileSources. ClientDataSource::clearData() not only removes the tiles generated from its stored features, but also removes the features themselves. This should definitely not happen when the scene changes. It's not clear why clearData() was used here at all, so I've simply removed the call.

The behavior of clearData() (which is to clear caches) was mistakenly conflated with the need to clear the set of features added to a ClientDataSource. These two behaviors are both necessary, but should be separate. So I've given ClientDataSource a new method called clearFeatures() that removes added features, and made its clearData() implementation fall back to that of the base class. So now when a scene is reloaded and clearData() is invoked, the ClientDataSource features won't be cleared. But a client can explicitly clear the features through MapData$clear.

This issue demonstrates how messy some of the TileSource abstraction is. A better, more complete solution would be to refactor these interfaces, but that will take more time.

Resolves #2117

Copy link

@sheikh-iffi sheikh-iffi left a comment

Choose a reason for hiding this comment

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

So far its working fine with this change. Will inform you guys, if faced with any issue regarding it.

@tallytalwar tallytalwar self-assigned this Dec 20, 2019
@tallytalwar tallytalwar self-requested a review December 20, 2019 06:09
@tallytalwar
Copy link
Member

Thanks @matteblair for getting on this.

This is an interesting behavior. I thought and confirmed that clearData was also responsible for updating the generation of the data source (which is then used in the tileManager to determine when to update the old source's tileSet.. and also in tileWorker to determine which tile task wins in priority).

I think this is going to cause issues during a scene update which is actually updating the datasource itself, and tileManager will continue to use old source (loaded) tiles, instead of throwing these away.

It might make sense to handle this in ClientDataSource::clearData overridden function, and have another function to do a clear of the features at appropriate scene update call (need to think more about when that would make sense, when a scene update changes the appropriate clientdatasource, maybe?)

@matteblair
Copy link
Member Author

I've reverted the original change and fixed this behavior in a different way now, so as to avoid unforeseen side effects with other data source types.

As @tallytalwar suggested, the ClientDataSource::clearData implementation was a necessary behavior to have available, but is distinct from TileSource::clearData which it overrides. Now ClientDataSource uses the base implementation for clearData (which doesn't remove previously added features) and has a separate clearFeatures method (which explicitly removes added features).

While I was at it, I also managed to clean up some of the glue code between ClientDataSource and MapData.

@tallytalwar
Copy link
Member

tallytalwar commented Dec 31, 2019

@matteblair looks great to me. Just a couple of clarifications:

  1. Can you update the description of the PR here, since we are still doing a clearData call when a scene is updated. So as it stands, clientdatasource will still be all "cleaned" up. If we have to give such control to the user, we can probably provide a flag with scene update, which will result in a call to clearFeature from clientDataSource::clearData. I am not sure if this is what @sheikh-iffi was running into, if so this PR won't solve it.
void ClientDataSource::clearData() {
     if (do_not_clear_data_but_feature_on_a_scene_update) {
          clearFeatures();
          return;
     }


    std::lock_guard<std::mutex> lock(m_mutexStore);
    std::lock_guard<std::mutex> lock(m_mutexStore);


    m_store->features.clear();
    m_store->features.clear();
    m_store->properties.clear();
    m_store->properties.clear();
    m_store->tiles.reset();

    m_generation++;
}
  1. I see you removed all mapPtr null checks, is it because these methods are syncronized with MapController, so such checks are immaterial?

@tallytalwar
Copy link
Member

This issue demonstrates how messy some of the TileSource abstraction is. A better, more complete solution would be to refactor these interfaces, but that will take more time.

In full agreement. I find it over-complicated, but I think this was added when we refactored some work for mbtiles. Also in agreement that its a much bigger task.

@matteblair
Copy link
Member Author

  1. Can you update the description of the PR here

Done - hopefully that makes the new behavior clearer. The original issue of ClientDataSource losing features when a scene is changed is still resolved.

  1. I see you removed all mapPtr null checks

I don't think I did? I removed some unused methods in MapController and changed some MapData methods to no longer call into MapController at all (which removed the need to check some nulls).

@matteblair
Copy link
Member Author

matteblair commented Jan 2, 2020

Added a few more small-ish related changes and rebased on master.

@matteblair matteblair force-pushed the clientsource-sceneupdate-fix branch from 895e9e1 to 00adea8 Compare January 2, 2020 22:36
@tallytalwar
Copy link
Member

Thanks for clearing all this up @matteblair. Merging.

@tallytalwar tallytalwar merged commit b48423b into master Jan 3, 2020
@tallytalwar tallytalwar deleted the clientsource-sceneupdate-fix branch January 3, 2020 18:15
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.

ClientDataSources removed after sceneupdate
3 participants