-
Notifications
You must be signed in to change notification settings - Fork 519
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
New Sample: Add custom dynamic entity data source #1269
New Sample: Add custom dynamic entity data source #1269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a really good start @duffh. Mostly code / sample simplification comments from me.
I think we should try to find a way to parse JSON using standard System.Text.Json.JsonElement
. That would allow us to get rid of the deserializer specific classes and simplify the sample. I can probably help with that if you need.
I also think we should opt for a static schema instead of trying to infer it from the file. I was testing with a bunch of different files, so that came in handy, but in a sample, we don't really need that. Up to you though.
I like the labelling and callout on the dynamic entity. It gives a sign that the entities from a custom source are first-class citizens in the API 👍
private Task? _processTask; | ||
private StreamReader? _streamReader; | ||
private CancellationTokenSource? _cancellationTokenSource; | ||
private TaskCompletionSource<object?>? _pauseTask; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're not actively controlling playback of the file, I think we can remove _pauseTask
altogether to simplify the code.
protected override async Task<DynamicEntityDataSourceInfo> OnLoadAsync() | ||
{ | ||
// Derive schema from the first row in the custom data source. | ||
var fields = await InferSchemaAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify, we can just return a static list of fields instead of inferring the schema.
var processed = await ProcessNextObservation(); | ||
|
||
// If the observation was not processed, continue to the next observation. | ||
if (!processed) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed an End-of-Stream bug here (my bug - sorry). We should check _streamReader.EndOfStream
and break out of the loop, or we'll run forever.
return fields; | ||
} | ||
|
||
public async Task StepAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this method if we're not managing file playback.
{ | ||
// Parse the observation from the file using the JsonGeoElementParser. | ||
// This will return a graphic holding a MapPoint and a dictionary of attributes. | ||
var graphic = JsonGeoElementParser.ParseGraphicFromJson(json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can find a different way to do the parsing so we're not deserializing into a Graphic
. I don't think we want to do that in a sample as DynamicEntityObservation
and Graphic
are not related (except that they both derive from GeoElement
) and we wouldn't want to confuse someone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing to a standard JsonElement
might be the ticket here. If we do that, we can probably get rid of the whole JsonGeoElement
which is kind of confusing in a sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, that simplifies things a lot.
// Create a new custom file source. | ||
// This takes the path to the desired data source and the field name that will be used as the entity id and the delay between each observation that is processed. | ||
// In this example we are using a json file as our custom data source. | ||
// This field name should be a unique identifier for each entity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our design discussions, there was some confusion about the field name
and the field value
. Probably best to be explicit about the field value being the unique identifier.
} | ||
} | ||
|
||
private async Task<List<Field>> InferSchemaAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd opt for removing this altogether and just using a static list of fields that's tied to the data.
|
||
namespace ArcGIS.Samples.AddCustomDynamicEntityDataSource | ||
{ | ||
public class CustomFileSource : DynamicEntityDataSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should rename this to something more specific to the data (i.e. SimulatedAisDataSource
or something similar). Then the sample could be less generic and more tightly coupled with the data. The ability to create data specific custom data sources is a reasonable message to give to users as well.
graphic.Geometry = GeometryEngine.Project(graphic.Geometry, SpatialReference); | ||
} | ||
|
||
// Add the observation to the dynamic entity layer for presentation on the map. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be careful the separation of the data source and the layer. Here we're adding an observation to the data source which can happen whether or not the data source is contained by a layer or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my end 👍. Just minor comments.
if (!processed) continue; | ||
|
||
// If the end of the file has been reached, break out of the loop. | ||
if (_streamReader.EndOfStream) _break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be moved above the !processed
check.
...les/Samples/Layers/AddCustomDynamicEntityDataSource/AddCustomDynamicEntityDataSource.xaml.cs
Outdated
Show resolved
Hide resolved
public string FilePath { get; } | ||
public string EntityIdField { get; } | ||
public TimeSpan Delay { get; } | ||
public SpatialReference SpatialReference { get; set; } = SpatialReferences.Wgs84; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify, we can probably remove this property since we're assuming WGS84 for this data source.
…aSource/AddCustomDynamicEntityDataSource.xaml.cs Co-authored-by: Greg De Stigter <[email protected]>
…hub.com/Esri/arcgis-maps-sdk-dotnet-samples into hduff/custom-dynamic-entity-data-source
xmlns:esri="using:Esri.ArcGISRuntime" | ||
xmlns:esriUI="using:Esri.ArcGISRuntime.UI.Controls"> | ||
<Grid> | ||
<esriUI:MapView x:Name="MyMapView" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WPF & WinUI: missing GeoViewTapped event subscriptions
Description
Added new sample to demonstrate how to display data from a custom dynamic entity data source using a dynamic entity layer.
Type of change
Platforms tested on
Checklist