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

Allow addProtocol in the worker code #3459

Merged
merged 10 commits into from
Dec 18, 2023
Merged

Allow addProtocol in the worker code #3459

merged 10 commits into from
Dec 18, 2023

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Dec 6, 2023

This allows adding a protocol in the worker thread by exposing an addProtocol method in the workers code and allowing to import a code into the workers.

One issue I noticed is that the raster source is not using the worker thread at all since all it does it fetch an image and convert it to pixels, so there's not much work in the works anyway, which means that if you add a protocol in the worker thread but use a raster source it won't be called.
This means that the developer needs to know where to add the method, which is OK, as the default is in the main thread and only if you'd like to optimize things you can move it to the worker thread.

The rest is the same.

I've also removed the import of the script when adding a source type, so that importing a script has only one entry point.

cc: @bdon, @msbarry

Launch Checklist

  • 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.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (f0028c9) 74.74% compared to head (2dae4d1) 74.74%.
Report is 16 commits behind head on main.

Files Patch % Lines
src/index.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3459   +/-   ##
=======================================
  Coverage   74.74%   74.74%           
=======================================
  Files         242      243    +1     
  Lines       19146    19154    +8     
  Branches     4240     4239    -1     
=======================================
+ Hits        14311    14317    +6     
- Misses       4835     4837    +2     

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

@msbarry
Copy link
Contributor

msbarry commented Dec 7, 2023

Interesting... at first glance this seems to make sense for the maplibre contour plugin since the entire thing (fetch DEM tile, calculate contours, encode vector tile) could happen inside a worker which would eliminate the need for it to manage it's own worker. But I'd also like for it to be able to share a DEM tile cache with raster-DEM source which happens back in the main thread, and to be able to use other protocols defined by plugins the user is using (ie. load raster DEM tiles from pmtiles). What do you think the best way to accomplish those 2 goals would be?

Also this approach requires you to know the internals of maplibre to know whether you need to inject a plugin into the main or worker thread (my diagram helps but it means we may break plugins if the fetch is moved between main/worker thread. I wonder if there's an approach where you can add a plugin that works no matter where the fetch happens? Either by requiring plugins provide both a worker and main thread implementation, or if the user just provides a worker or main thread implementation then maplibre falls back to sending a request to where the protocol is implemented?

@HarelM
Copy link
Collaborator Author

HarelM commented Dec 7, 2023

To overcome this limitation, I think I can think of a way to register the worker to getResource actor message so that the main thread can try and call the worker thread so that it doesn't matter where you registered the protocol - there's a need to make sure this doesn't cause an endless loop, but I think it's possible without a lot of changes - this will reduce the need to know where to add the protocol, and you can fine tune the performance if you decide to (opt-in kind of performance).

Regarding shared tiles, this is a different story and I'm not sure this is related to this PR, we also have the same issue with the terrain and hillshding - i.e. you have to use different sources so that things are not colliding (tile fetch aborting etc). I'm not sure I have an easy solution for this...

@HarelM
Copy link
Collaborator Author

HarelM commented Dec 7, 2023

Passing a actor to the makeRequest method when in the main thread proved harder than I though...
I'm not sure I have a good solution for this ATM, and the more I look at it the less I'm convinced this is the right direction.
I'll try again and see if I can find an elegant solution, but I think that the need to know where to run your code is problematic and kinda a deal breaker from my perspective as a developer.
IDK... I'll sleep on it.

@HarelM
Copy link
Collaborator Author

HarelM commented Dec 10, 2023

I had the following idea:
Instead of creating a public API to add a protocol to the worker thread I can let the plugin owner decide on the implementation while giving them all the relevant building blocks by providing the following two new APIs:

  1. The ability to load a script in the worker thread
  2. The ability to add a protocol method in the worker thread

This way it is up to the developers to properly test everything and make sure it's working as expected instead of trying to solve everything for them.

I'll change this PR so you can take a look at it.

@HarelM HarelM marked this pull request as ready for review December 10, 2023 11:01
@HarelM HarelM requested a review from neodescis December 10, 2023 11:11
@msbarry
Copy link
Contributor

msbarry commented Dec 10, 2023

I think that makes more sense. Seems like from a users perspective they just want to say maplibregl.addPlugin(plugin) then the contract is between the plugin writer and maplibre. It seems like this moves us more in that direction, or maybe if the api is plugin.addTo(maplibregl) then we just need to expose APIs like this for the plugin to hook into.

@HarelM HarelM requested review from wipfli and birkskyum December 10, 2023 18:49
@HarelM
Copy link
Collaborator Author

HarelM commented Dec 10, 2023

I guess a guide about how to write a plugin that uses these abilities would create a common plugins look and feel, hopefully. Generally speaking I'm not sure what API a plugin should implement, so I'm not sure how to define this API.
Also there are a lot of types of plugins, so I'm not sure all should conform to the same API.
@msbarry let me know if you think the code in it's current state is OK and I'll merge it.

Thanks again for the input!

@HarelM
Copy link
Collaborator Author

HarelM commented Dec 17, 2023

@birkskyum @neodescis can you guys approve this?
I would like to merge this to main.

@HarelM HarelM merged commit 190f782 into main Dec 18, 2023
14 checks passed
@HarelM HarelM deleted the add-protocol-in-worker branch December 18, 2023 17:19
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.

4 participants