-
Notifications
You must be signed in to change notification settings - Fork 0
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
#6933: Enhance IconSymbolizer and Contrast enhancement #1
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -359,6 +359,10 @@ export interface IconSymbolizer extends BasePointSymbolizer { | |
* A path/URL to the icon image file. | ||
*/ | ||
image?: string; | ||
/** | ||
* The format (MIME type) of the image provided. | ||
*/ | ||
format?: `image/${'png' | 'jpg' | 'jpeg' | 'gif' | 'svg+xml'}`; | ||
/** | ||
* If true, the icon will be kept upright. | ||
*/ | ||
|
@@ -549,12 +553,22 @@ export interface ColorMap { | |
extended?: boolean; | ||
} | ||
|
||
/** | ||
* A VendorOption defines the algorithm to apply for Normalize contrast enhancement. | ||
*/ | ||
export interface VendorOption { | ||
algorithm?: 'StretchToMinimumMaximum' | 'ClipToMinimumMaximum' | 'ClipToZero'; | ||
minValue?: number; | ||
maxValue?: number; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. VendorOption seems a generic name but in this case is used only for ContrastEnhancement and it has a specific structure that match what the GeoServer expect. Probably we should make this more specific: export interface ContrastEnhancementVendorOption {
algorithm?: 'StretchToMinimumMaximum' | 'ClipToMinimumMaximum' | 'ClipToZero';
minValue?: number;
maxValue?: number;
} export interface ContrastEnhancementAlgorithm {
name?: 'StretchToMinimumMaximum' | 'ClipToMinimumMaximum' | 'ClipToZero';
minValue?: number;
maxValue?: number;
}
export interface ContrastEnhancement {
enhancementType?: 'normalize' | 'histogram';
gammaValue?: number;
algorithm?: ContrastEnhancementAlgorithm;
} or more generic to be used in other context export interface VendorProperty {
name?: string;
options?: any;
} From the previous code it seems they prefer specific naming. My suggestions were more about the meaning of VendorOption itself that in this context seems to generic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We agreed to change only the name of VendorOption to ContrastEnhancementVendorOption |
||
|
||
/** | ||
* A ContrastEnhancement defines how the contrast of image data should be enhanced. | ||
*/ | ||
export interface ContrastEnhancement { | ||
enhancementType?: 'normalize' | 'histogram'; | ||
gammaValue?: number; | ||
vendorOption?: VendorOption; | ||
} | ||
|
||
/** | ||
|
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 see that you are using nullable types for all the properties. If any of this is required I would remove the question mark.
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.
For example, it is strange to see a sum type (alghoritm) as nullable, I would add a 'None' type if no alghoritm is possible, and remove the nullable
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 the algorithm property alone is required. So I will update the property accordingly