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

Feature/70 add wcslayer #204

Merged
merged 42 commits into from
Aug 7, 2023
Merged

Feature/70 add wcslayer #204

merged 42 commits into from
Aug 7, 2023

Conversation

seahro
Copy link
Collaborator

@seahro seahro commented Jul 24, 2023

Closes #70
Adds WCS Layer class and demo page
Subitems required to be created
-Creates the Raster Stretch Renderer class and methods
-DimensionalDefinitions class and methods for imagerylayers with mosaics

@seahro seahro self-assigned this Jul 24, 2023
});
let wcsLayer = newLayer as WCSLayer;
newLayer = wcsLayer;
copyValuesIfExists(layerObject, wcsLayer, 'bandIds', 'copyright', 'coverageId', 'coverageInfo', 'customParameters', 'fields', 'interpolation', 'maxScale', 'minscale', 'multidimensionalDefinition', 'rasterInfo', 'renderer');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to use the jsBuilder here rather then the copy values for the render and mulidimensionDefinition

@seahro seahro marked this pull request as ready for review August 2, 2023 19:19
Copy link
Collaborator

@TimPurdum TimPurdum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roscoe, excellent work getting this all functioning! I'm giving you a lot of tidying feedback, and I would encourage you to review your own code more thoroughly (using a diff between develop and your own branch such as the tool in Rider, or using github.dev on the PR before requesting review), to catch more of these up front.

Also have a bit more work to support MapComponent inheritance for ColorRamp.

/// The url for the WCS Layer source data.
/// </param>

public WCSLayer(string? url = null, PortalItem? portalItem = null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add List<DimensionalDefinition> multidimensionalDefinition, RasterStretchRenderer renderer, double opacity, string title to this constructor. Use the constructor in your WCSLayers.razor samples page instead of setting these properties directly. In fact, I think we should make these private set; properties (only for the ones that are not also [Parameter]), because if you set them after the layer is rendered, it will break.

public WCSLayer()
{
}
/// <summary>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be a space before these comments, and not one after them, for consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected across all the class files.

<a class="btn btn-primary" target="_blank" href="something">NOAA Sea Surface Temperature Charts</a>
</div>
<Label>Add the sample WCS Layer URL to see a visualization of global sea surface temperature data with or without color rendering using the RasterStretch:</Label>
<InputText @bind-Value="_wcsLayerUrl"></InputText>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that we really need this input field. I doubt anyone is going to test their own layers in our samples page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed. I think for some samples it makes sense to have the input field, others really dont need it.

private bool _visible = false;
private bool _mapRendered = false;

private string _wcsLayerUrl2 = "https://sampleserver6.arcgisonline.com/arcgis/services/ScientificData/SeaTemperature/ImageServer/WCSServer";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is never used and can be deleted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an oversight while I was cleaning up the file...corrected.

private MultipartColorRamp? multipartColorRamp;
private MapView _view;
private bool _markup = false;
private bool _visible = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected

@@ -72,6 +73,7 @@ public enum RendererType
{
#pragma warning disable CS1591
Simple,
UniqueValue
UniqueValue,
RasterStretch
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be removed and not used, since we discovered there is no inheritance. You could replace with just a string entry.

wcsLayer.multidimensionalDefinition.push(wcsMDD);
}
}
copyValuesIfExists(layerObject, 'bandIds', 'copyright', 'coverageId', 'coverageInfo', 'customParameters', 'fields', 'interpolation', 'maxScale', 'minscale', 'rasterInfo');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking through these, I wonder if fields is the same type as used in other places. We do have a buildJsField method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be. These were not essential to the building the sample page and following the pattern from other classes, these were included based on the properties in the rasterstretchrenderer

@@ -656,4 +667,56 @@ export function buildDotNetTimeExtent(timeExtent: any): any | null {
start: timeExtent.start.toISOString(),
end: timeExtent.end.toISOString()
} as any;
}

export function buildDotNetDimensionDefinition(dimensionDefinition: DimensionalDefinition): any | null {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think any of these methods are actually used. The use case for a dot net builder would be if the data was loaded from a server via JS, and then needed converting back to .NET. If there's no calling code, I would delete these (sorry).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed these and the imports at the top of the file.

return multidimensionalDefinition;
}

export function buildJSMultipartColorRamp(dotNetMultipartColorRamp: any): MultipartColorRamp | null {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase s in Js

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected in all spots

@@ -22,6 +22,7 @@
<GeneratePackageOnBuild Condition="$(Configuration) == 'RELEASE'">true</GeneratePackageOnBuild>
<TargetFrameworks>net7.0;net6.0</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<UserSecretsId>364f0380-f3fa-441e-8138-e888b38a26c4</UserSecretsId>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user secrets definitely don't work in a library project

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

Copy link
Collaborator

@TimPurdum TimPurdum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need some extra [Parameter]s marked and remove some extraneous constructors!

{
public ColorRamp() { }

public ColorRamp(ColorRampType colorRampType, MultipartColorRamp colorRamps)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

abstract classes don't need constructors at all, unless you're going to call : base() from the inheriting class and pass values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected and removed. noted for future classes

/// The dimension associated with the variable..
/// </summary>
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public string? DimensionName { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These properties should be [Parameter] in this class

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected

/// <summary>
/// The computeGamma automatically calculates best gamma value to render exported image based on empirical model. This is applicable to any stretch type when useGamma is true.
/// </summary>
public bool? ComputeGamma { get; private set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything not set via RegisterChildComponent should be a [Parameter]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected, once the constructor was refactored, these should have been corrected.

@using System.Text.Json
@using dymaptic.GeoBlazor.Core.Components.Renderers.ColorRamps
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to put this in alphabetically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleaned up

@seahro
Copy link
Collaborator Author

seahro commented Aug 7, 2023

Adjusted the wcs sample page and classes to adjust to the new publish build effects of "warnings as errors". tested and operational

@TimPurdum TimPurdum merged commit d1c451f into develop Aug 7, 2023
@TimPurdum TimPurdum deleted the feature/70_add_wcslayer branch August 7, 2023 18:33
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.

Add WCSLayer
3 participants