Add Feature Properties Transform #4199

wipfli · 2024-05-29T13:02:24Z

Proof-of-concept implementation for #4198. The implantation is inspired the addProtocol implementation.

Demo:

/~https://github.com/wipfli/maplibre-feature-properties-transform-example

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.

wipfli · 2024-06-17T06:51:22Z

This is ready for review. Let me know what needs to be changed

src/source/protocol_crud.ts

src/source/worker.ts

src/source/worker_tile.ts

src/util/config.ts

HarelM · 2024-06-17T12:21:26Z

Thanks again for the PR!
Can you please resolve conflicts and also create some tests and an example page of how to use this feature?

wipfli · 2024-06-28T17:45:54Z

For the tests I will need some help. Do we already have tests for maplibregl.importScriptInWorker()?

HarelM · 2024-06-28T19:14:21Z

Import script requires integration tests, so I wouldn't start with that.
Try unit testing the worker and the tile parser by mocking and spying a lot if the stuff.
Also, don't forget the changelog entry.

HarelM · 2024-07-01T20:23:03Z

I've opened the following PR:
wipfli#341
Hope it helps...

* Alternative option * Improve agronomics, add test * Remove unwanted change

codecov-commenter · 2024-07-02T17:41:37Z

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.79%. Comparing base (b8cb683) to head (2b2d199).
Report is 578 commits behind head on main.

Files with missing lines	Patch %	Lines
src/source/worker.ts	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4199      +/-   ##
==========================================
- Coverage   87.85%   87.79%   -0.07%     
==========================================
  Files         246      246              
  Lines       33372    33392      +20     
  Branches     2182     2183       +1     
==========================================
- Hits        29318    29315       -3     
- Misses       3064     3084      +20     
- Partials      990      993       +3

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

HarelM

See my comments at:
wipfli#342

Seems like this feature needs a bit more work.

* Alternative option * Improve agronomics, add test * Remove unwanted change * Update browser.test.ts - fix test * Update land.json

HarelM · 2024-07-10T13:37:29Z

The image is empty, I think it's a bug in chrome or headless chrome... not sure.
I think the following arguments may help:
/~https://github.com/maplibre/maplibre-gl-js/pull/4299/files

Or you can toggle headless for image creation.

HarelM · 2024-07-10T13:38:56Z

src/util/web_worker.ts

@@ -8,6 +9,8 @@ export interface WorkerGlobalScopeInterface {
    registerRTLTextPlugin: (_: any) => void;
    addProtocol: (customProtocol: string, loadFn: AddProtocolAction) => void;
    removeProtocol: (customProtocol: string) => void;
+    getFeaturePropertiesTransform: () => FeaturePropertiesTransform;


I think get is not needed anymore, right?

HarelM · 2024-07-10T13:39:45Z

test/unit/lib/web_worker_mock.ts

@@ -11,6 +11,8 @@ export class MessageBus implements WorkerGlobalScopeInterface, ActorTarget {
    registerRTLTextPlugin: any;
    addProtocol: any;
    removeProtocol: any;
+    getFeaturePropertiesTransform: any;


HarelM · 2024-07-10T13:41:07Z

Now you have a failing test that reproduces what I said :-)

louwers

I didn't know that you could send some script to be executed to a worker like this! I am intrigued.

Here it says that after using createObjectURL, the URL needs to be cleaned up again with revokeObjectURL to avoid a memory leak: https://developer.mozilla.org/en-US/docs/Web/API/URL/createObjectURL_static

Since the script is only executed once, I think MapLibre GL JS should take care of this cleanup after the script is executed.

Maybe importScriptInWorkers can be updated to also take a function with (self: ServiceWorkerGlobalScope) => void as prototype. This has the advantage that instead of writing the script as a string, you get type and syntax checking. Also, you don't need to repeat yourself to prepare the data URL and the cleanup can be handled automatically.

If a function is passed to importScriptInWorkers, you would call toString(), strip the resulting string to get the function body, and then use decodeURIComponent, Blob and URL.createObjectURL internally.

Modify the MessageType.importScript to pass along an optional boolean to indicate that revokeObjectURL needs to be called on the worker side once the script has been executed.

HarelM · 2024-07-11T05:18:55Z

The example is a way to inline everything into a single file for demo purposes, I would not advise to use createObjectURL in production, but instead use a javascript file stored next to the code or even on a CDN. This file can be created using the regular typescript tooling to support type check and everithing.
I wouldn't like to complicate the import script interface for this edge case.
This is also outside the scope of this PR I believe.

louwers · 2024-07-11T10:40:23Z

Ah OK, I thought that was the recommended approach. Makes sense. 👍

wipfli · 2024-07-30T15:00:04Z

The documentation for queryRenderedFeatures says that the result "contains the properties of its source feature":

maplibre-gl-js/src/ui/map.ts

Line 1519 in a942081

    
                * The `properties` value of each returned feature object contains the properties of its source feature. For GeoJSON sources, only

Based on this I would suggest to add a note to the documentation of the setFeaturePropertiesTransform function that changes will not be reflected in queryRenderedFeatures. For tests I would suggest to write some render tests. What do you think?

HarelM · 2024-07-30T18:51:52Z

I think that you'll have a hard time creating render tests for this feature as render tests are mainly style driven and this feature "breaks" the style.
Browser integration tests are enough.

Can you elaborate on the limitation of query render features? If this hook is creating a discrepancy between the rendered results and the returned value of the query it might confusing to users, wouldn't it?

wipfli · 2024-08-27T07:50:50Z

The functionality proposed in here seems to have a lot of overlap with feature-state. It is just global on all features and not on individual features. I will think a bit if there is a better api than what is proposed here...

HarelM · 2024-11-26T21:51:35Z

@wipfli is this still something you want to push forward?
Are any breaking changes needed while we are at version 5 pre-release for this feature?

wipfli · 2024-11-28T07:31:55Z

Thanks for the ping @HarelM.
I think this would be a nice feature but I have not been thinking about it for a while. The main open question was the result from queryRenderedFeatures right?
Regarding v5, this pull request should not lead to breaking changes so it can come after

HarelM · 2024-11-28T08:47:28Z

Yes, I believe the issue was around queryRenderedFeatures.
Thanks for the update.

neodescis · 2025-01-17T21:50:34Z

Closing in favor of #5370

Add Feature Properties Transform

dbde2b5

wipfli marked this pull request as draft May 29, 2024 13:02

wipfli mentioned this pull request May 29, 2024

Feature Properties Transform #4198

Closed

wipfli added 4 commits June 4, 2024 23:01

Remove index.ts export, add featureID

cb352f2

Add style.json

2729f17

async

9d7122e

Remove style.json

f74bc8c

wipfli marked this pull request as ready for review June 17, 2024 06:51