Use only bounds for geojson diff tile reload

[HarelM]

Launch Checklist

• Fixes geoJsonSource.updateData not working properly after updating to 5.13.0 #6795

This changes the logic of tile refreshing for geojson
The idea behind this fix:

1. When updating the data, we know exactly which geometries were changed, so we use this information to return the relevant bounds of these geometries
2. It simplifies to only using bounds and not using ids of previous features

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• Write tests for all new functionality.
• Add an entry to CHANGELOG.md under the ## main section.

[HarelM]

@lucaswoj @wayofthefuture let me know what you think about the direction of this PR before I invest more time in fixing the tests and adding new tests.

[codecov]

Codecov Report

❌ Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.42%. Comparing base (d103e4f) to head (b37f167).

Files with missing lines	Patch %	Lines
src/source/geojson_source.ts	95.45%	1 Missing ⚠️
src/source/geojson_worker_source.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6800      +/-   ##
==========================================
- Coverage   92.42%   92.42%   -0.01%     
==========================================
  Files         288      288              
  Lines       23809    23814       +5     
  Branches     5058     5058              
==========================================
+ Hits        22005    22009       +4     
- Misses       1804     1805       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wayofthefuture]

I like the way this is headed.

However, let's say a feature was moved from 1 tile to another tile. How do you know to invalidate the previous tile without the feature id? The new bounds would not affect the previous tile.

[HarelM]

If a feature was moved from one tile to the other - I have the previous geometry just before I apply the update, and the new geometry.
So I "invalidate" tiles that have one of those geometries.

[HarelM]

I've gone and updated the relevant tests. I've removed some tests that are "duplicated" in the sense that they are being tested in lat lng bounds and there's no need to retest them in geojson source.
I've added tests that are using only public API to mimic how the class is behaving.
There's still a weird issue with the "covering world tile", which causes a test to fail.
I added a similar test to lnglatbounds tests, which also fails to highlight the issue in a simpler test.
@lucaswoj, do let me know what you think about this test and if there's something wrong about it.

[lucaswoj]

Please hold off on merging these changes until I have time to review later today. I think this introduces some new bugs.

[HarelM]

Sure, take your time. I wouldn't merge this before you review it.

[lucaswoj]

Removing this codepath will cause performance regressions in some cases. Although having two codepaths is ugly, invalidating tiles by id is much faster and more accurate than doing so by bounds.

[lucaswoj]

Thanks for diving into this code @HarelM! This does fix the bug but it may regress performance in some cases. I've proposed another fix at #6801, ready for your review, that is performance neutral.

[HarelM]

Can we measure how much performance degradation this is causing?
This code is a lot simpler to maintain and doesn't relay on feature index (including the timing related to it, which I find hard to make sure won't break in the future).

[lucaswoj]

Can we measure how much performance degradation this is causing?

There's not a single number that captures the whole picture.

We can say that #6562 would no longer apply to sources loaded from a URL, so updating 1 point out of 100k points would become ~100% slower after this PR merges.

#6562 would also no longer apply in some cases involving more complex geometries, like an L-shaped line that crosses three out of 4 tiles in the viewport. We could create another benchmark that would become ~100% slower after this PR merges, for a non-URL-backed source.

[HarelM]

Fair enough, can you take a look at the lat lang bounds failing test? I'm not sure I understand why it's failing.

[HarelM]

@mwilsnd does the fix related to geojson undefined properties has to do with the feature index etc?
I'm wondering if the fix here is helps with your fix or if they are not related.

[mwilsnd]

Take a look: #6803 - this just changes how the layers are constructed in the feature index for geojson sources, I'm not sure if this is fully related.

[HarelM]

The features from the geojson are sent back for every tile, so it might be somewhat related, I'm not sure, I need to wrap my head around it...

[wayofthefuture]

After giving this a look I can say that I definitely prefer this solution. Considering it's a geojson source, and geojson supports string ids, the fact that this also supports string ids just makes sense. It also saves us (and many) a breaking refactor to eliminate the warning. (correct me if I'm wrong).

Also, @HarelM's implementation is similar to my implementation used in the differential GeoJSON-VT PR that is pending. In this PR, updates are applied to the source and affected features are returned as well. Not sure if that would benefit in the future, but I like the idea of both sides using similar methods.

[HarelM]

Thanks for the review!
Note that I updated my initial message as feature updates were supported in the previous version, I just read the code wrong.

There are some performance changes (in some flows they are improved while in others they degrade).
I can't say which is better.
I can say that I think this approach is a bit less fragile as it doesn't relay on feature index.

I'll work on the review comments nevertheless.

[wayofthefuture]

#6809

[HarelM]

Let me know if anything else is needed before merging this.

[wayofthefuture]

Awesome!