-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[GEOT-6503] Extend SLD to support background fill specification at the UserStyle level #2775
Conversation
|
||
/** The background Fill , if any, <code>null</code> otherwise */ | ||
public default Fill getBackground() { | ||
return null; |
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.
exciting you went for Fill
if (firstStyle.getBackground() != null) { | ||
Paint background = styleFactory.getPaint(firstStyle.getBackground(), null, null); | ||
graphics.setPaint(background); | ||
graphics.fill(paintArea); |
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.
Do we need to make the image transparent, before applying your fill, in case the fill has some transparency effects?
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.
No, StreamingRenderer is given a Graphic2D, it should not alter what was done before on it (could have been used for overlay on top of other stuff for example). It should be simple and predictable, just do what it was asked to do (apply the background).
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.
thanks that makes sense
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 to me. A fill definitely sounds like a good plan.
a4409d9
to
8daa5d3
Compare
@ianturton @jodygarnett docs and tests completed, ready for actual review. |
…e UserStyle level
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 to me.
I have just reported a trivial typo, but is not a blocker for the approval :-)
StyledLayerDescriptor sld, | ||
BackgroundMBLayer background, | ||
FilterFactory2 ff) { | ||
// Background does not use a source; construct a user later with a world extent |
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.
just a minor TYPO here... layer instead of later
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 checked this out earlier and am keen to see it go ahead.
Andrea are you in position to update the checklist and merge?
SLD background support with MBStyle leveraging it to get simpler, more reliable background layer.
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for unsupported modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.