OGC Features Provider and Features Layer #12493

jjspace · 2025-02-24T19:42:58Z

Description

Opening as a draft for early review and feedback around implementation. Will fill out the description more later.

See #12456

Also worth taking a look at @ggetz's feature-prototype branch for how we might do some of the lower level stuff.

Issue number and link

Resolves #12456

Testing plan

There's a new development sandcastle for the OgcFeatureProvider and FeatureLayer classes

Author checklist

I have submitted a Contributor License Agreement
I have added my name to CONTRIBUTORS.md
I have updated CHANGES.md with a short summary of my change
I have added or updated unit tests to ensure consistent code coverage
I have updated the inline documentation, and included code examples where relevant
I have performed a self-review of my code

github-actions · 2025-02-24T19:43:21Z

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

ggetz · 2025-02-25T16:04:57Z

packages/engine/Source/DataSources/FeatureProvider.js

+ * @param {Object} options
+ */
+function FeatureProvider(options) {
+  this.id = options.id;


We've had some pain points with inheritance in the past. To avoid these, I would recommend treating FeatureProvider as an interface as much as possible. That is, I would avoid defining any implementation details, and leave it to the implementor or static utility-like functions.

ggetz · 2025-02-25T16:11:54Z

packages/engine/Source/DataSources/OgcFeatureProvider.js

+    // this._itemsLoaded < this.maxItems
+    itemsLoadedThisRequest < this.maxItems
+  ) {
+    if (localAbortController.signal.aborted) {


To clarify my understanding, the abort controller as it is used in this class is not actively cancelling an in-progress requests— Only preventing more requests from happening. Is that correct?

As a follow up question, is this something important we need to consider for the Resource implementation? Would it fit in with the existing implementation, or would it necessitate more significant refactoring?

To clarify my understanding, the abort controller as it is used in this class is not actively cancelling an in-progress requests— Only preventing more requests from happening. Is that correct?

Correct, as currently written this just stops the loop and future requests from starting and ignores the results. I implemented this first and it basically is just acting as a nicer wrapper for toggling a boolean for previous calls to this function. I added the cancelResource function later to try and actually handle canceling the underlying network requests themselves and prevent the browser itself from blocking. There's probably a cleaner way to do this but "it worked" for now.

The AbortController and signal is designed to actually be used with the native fetch (I picked up the pattern when working on a vscode extension). fetch has an option to pass the signal in when it's created and it will just handle canceling the underlying request. This won't work for us because Resource is still built around XHR requests which do not have this functionality.

Makes sense. I'd still hear any thoughts around the existing Resource implementation. Are we at the point where it's enough of a blocker that we should be considering a refactor?

I somehow missed that you can addEventListener to an AbortSignal. I'm pretty sure this lets us augment Resource/Request to take a signal parameter like fetch can and have it cancel the requests when the signal is aborted. I've implemented this for a few of the Resource.fetch** calls in my latest commits and it works. Take a look and see if this makes sense to you then I can do the rest.

It may even make sense to do a dedicated PR for this update to Resource that I can then pull into this branch, open to ideas

It may even make sense to do a dedicated PR for this update to Resource that I can then pull into this branch

That sounds like a great plan to me 👍

I also saw that your changes may help us remove the need to use defer.js.

That file was a compromise from the when to promise refactor, and we'd like to get rid of it ASAP. I think Resource, RequestScheduler, and KmlDataSource are the last bastions of using defer.js.

ggetz · 2025-02-25T16:25:07Z

packages/engine/Source/DataSources/FeatureProvider.js

+
+Object.defineProperties(FeatureProvider.prototype, {});
+
+FeatureProvider.prototype.requestFeatures = function (bbox, time) {


I'm wondering if the bbox parameter should be a bit more generic. While bbox is very straightforward for the OGC features API, it may not map to other providers as well such as something more tile-based.

I'm trying to think through what may be needed to determine the proper request... Perhaps camera (position, direction, and frustum extents), scene mode, ellipsoid... I'm trying to avoid something as low-level as passing in the FrameState as we do in many places, but that may be a possibility.

As discussed in person I believe this will be replaced with the full camera object so any implementations can calculate a bbox or other features from that.

ggetz · 2025-02-25T16:26:40Z

packages/engine/Source/DataSources/FeatureLayer.js

+/**
+ * A class for things
+ * @constructor
+ * @param {FeatureProvider | OGCFeaturesProvider} provider


If OGCFeaturesProvider implements FeatureProvieder, I don't think we'd need to specify the type explicitly, no?

Technically no. However as previously discussed at length TS will not recognize inheritance that's defined purely through JSDoc so it doesn't understand this without both. I have been adding JSDoc comments for types to help me while I develop but may need to do a pass on all of them before this is merged to clean them up to be "correct"

ggetz · 2025-02-25T16:28:12Z

packages/engine/Source/DataSources/FeatureLayer.js

+    ),
+  );
+
+  if (distance > 400) {


There are camera moved events on Camera.js. Perhaps they are sufficient?

I took a look at the move events but wasn't sure that relying on moveEnd was the right play here. I didn't dig into it too much yet though.

That said, I pushed up logic for tracking what tiles have been requested from the provider level. They keep track of which ones are active and if they're already active it doesn't try to re-request them. I think this may work well enough to prevent restarting requests that are still visible making this sort of check less important. Let me know your thoughts?

packages/engine/Source/DataSources/OgcFeatureProvider.js

ggetz · 2025-02-25T16:34:43Z

packages/engine/Source/DataSources/FeatureLayer.js

+/**
+ * @param {Feature} feature
+ */
+FeatureLayer.prototype.add = function (feature) {


We'll probably want remove and removeAll functions as well.

ggetz · 2025-02-25T16:37:30Z

packages/engine/Source/DataSources/FeatureLayer.js

+  // this._primitives = this._dataSource.entities;
+  this._primitives = [];
+
+  this.features = options.features ?? new AssociativeArray();


I couldn't find an issue for it, but AssociativeArray can probably be replaced with native Map at this point. It's easy an efficient to pull the values as an array through built-in methods.

Also, and perhaps more importantly, we should be smart about how we store data for lots and lots of features. There's potential for a lot of duplication across the graphical representation (whether than be Entities or Primitives) and a raw array. We likely want to release the raw features once the representations have been created.

ggetz · 2025-02-25T16:39:00Z

packages/engine/Source/DataSources/FeatureLayer.js

+  this.id = options.id;
+  // this.name = options.name;
+  this._show = options.show ?? true;
+


Since this defines a Rectangle, we should likely be specifying an ellipsoid at this granularity as well so it's meaningful to map the extents to worldspace positions.

ggetz · 2025-02-25T16:43:04Z

packages/engine/Source/DataSources/parseGeoJson.js

+ *
+ * @param {GeoJson} geoJson
+ * @param {ParseOptions} [options]
+ * @returns {Entity | Entity[] | undefined}


Do you think it would be possible to map this to a list of Feature objects? And that, in turn, those could be mapped to Entities in another function to be used GeoJsonDataSource?

That way, the future implementation could map the returned features to primitives.

ggetz · 2025-02-25T18:26:40Z

packages/engine/Source/DataSources/FeatureLayer.js

+  // this.name = options.name;
+  this._show = options.show ?? true;
+
+  this.rectangle = options.rectangle ?? new Rectangle();


Is this rectangle intended to be the extent of the data, or a user defined restriction?

I think these should be separate properties, and the former should probably be read-only via the public API.

ggetz · 2025-02-25T18:28:56Z

packages/engine/Source/DataSources/FeatureProvider.js

+ */
+function FeatureProvider(options) {
+  this.id = options.id;
+  this.credit = undefined;


This should be implemented such that when the data is in view, credits are added on screen or in the lightbox.

I'm not sure what you plan was here, so I'm just stating this explicitly.

…racking

jjspace · 2025-03-04T21:46:46Z

@ggetz I just pushed up the logic for some basic "tiling" of an OGC feature provider to try and break up the area we request. It also tracks which tiles have already been requested to avoid excess requests for areas that are still visible or have been completely loaded already. This seems to help reduce the quantity of requests dramatically. I still have some open questions I need to try and address but take a look at the last commit and see if my current approach is making sense so far.

Still need to figure out how to decide what level to request
Need to check how this handles services that do not return extents which will basically mean tiling the whole world
General question? Do we have functions to get a list of "parent" or "child" tiles from a Tiling Scheme?
- I'd like to be able to check if a parent tile is already fullyLoaded or if all children are fullyLoaded to try and avoid loading the same data twice.

I also responded to some of the comment threads above so please check those too

… ones

jjspace added 10 commits January 31, 2025 12:26

feature service exploration sandcastle

3fb9c54

prevent duplicates option in GeoJsonDataSource

f44c30b

naive pagination and camera bbox

b6d377b

page loading using next, more finland data

337c195

add iTwin provider, fix abort signal

23cc6b3

fix pagination

28647ca

Merge remote-tracking branch 'origin/main' into ogc-features

b6d0a18

Merge remote-tracking branch 'origin/main' into ogc-features

a664d7c

adjust duplicate prevention for multipolygon

6dc711b

early ogc feature provider implementation

6ec428f

jjspace requested a review from ggetz February 24, 2025 19:43

ggetz reviewed Feb 25, 2025

View reviewed changes

packages/engine/Source/DataSources/OgcFeatureProvider.js Outdated Show resolved Hide resolved

ggetz reviewed Feb 25, 2025

View reviewed changes

jjspace added 4 commits February 28, 2025 15:49

Merge remote-tracking branch 'origin/main' into ogc-features

59374da

some jsdoc changes

1056f55

Merge remote-tracking branch 'origin/main' into ogc-features

675f3ef

load data with events, load based on tile rects for consistency and t…

279502e

…racking

Merge remote-tracking branch 'origin/main' into ogc-features

a178731

jjspace added 3 commits March 13, 2025 15:41

Merge remote-tracking branch 'origin/main' into ogc-features

97e2366

dynamic tile level, support Points, track empty tiles instead of full…

154f3bd

… ones

Merge remote-tracking branch 'origin/main' into ogc-features

1f9580c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OGC Features Provider and Features Layer #12493

OGC Features Provider and Features Layer #12493

jjspace commented Feb 24, 2025

github-actions bot commented Feb 24, 2025

ggetz Feb 25, 2025

ggetz Feb 25, 2025

jjspace Feb 28, 2025

ggetz Mar 3, 2025

jjspace Mar 4, 2025

ggetz Mar 5, 2025

ggetz Mar 5, 2025

ggetz Feb 25, 2025

jjspace Feb 28, 2025

ggetz Feb 25, 2025

jjspace Feb 28, 2025

ggetz Feb 25, 2025

jjspace Mar 4, 2025

ggetz Feb 25, 2025 •

edited by jjspace

Loading

ggetz Feb 25, 2025

ggetz Feb 25, 2025

ggetz Feb 25, 2025 •

edited

Loading

ggetz Feb 25, 2025

ggetz Feb 25, 2025

ggetz Feb 25, 2025

jjspace commented Mar 4, 2025


		Object.defineProperties(FeatureProvider.prototype, {});

		FeatureProvider.prototype.requestFeatures = function (bbox, time) {

OGC Features Provider and Features Layer #12493

Are you sure you want to change the base?

OGC Features Provider and Features Layer #12493

Conversation

jjspace commented Feb 24, 2025

Description

Issue number and link

Testing plan

Author checklist

github-actions bot commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz Feb 25, 2025 • edited by jjspace Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjspace commented Mar 4, 2025

ggetz Feb 25, 2025 •

edited by jjspace

Loading

ggetz Feb 25, 2025 •

edited

Loading