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.

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?

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

packages/engine/Source/DataSources/OgcFeatureProvider.js

+OgcFeatureProvider.prototype.requestFeatures = async function ({
+  bbox,
+  time,
+  partialLoadCallback,


This should probably be defined in FeatureProvider even if it's a no-op for certain implementations.

Also consider exposing events on FeatureProvider like featuresLoaded which can be subscribed to by the FeatureLayer.

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.

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

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

59374da

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 Feb 25, 2025

jjspace Feb 28, 2025

ggetz Feb 25, 2025

jjspace Feb 28, 2025

ggetz Feb 25, 2025

ggetz Feb 25, 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


		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

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

ggetz Feb 25, 2025 •

edited by jjspace

Loading

ggetz Feb 25, 2025 •

edited

Loading