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

RFC 138 Implementation - Reference SLD files in Mapfiles #7034

Merged
merged 5 commits into from
Mar 13, 2024

Conversation

geographika
Copy link
Member

Pull request for https://mapserver.org/development/rfc/ms-rfc-138.html - allowing SLD files to be referenced in a Mapfile:

LAYER
  STYLEITEM "sld://mysldfile.xml"
  ...
END

The logic of applying the LAYER created from the SLD to the MAP LAYER was moved to a new function msApplySldLayerToMapLayer. This allows the code to be reused to set a default style from the SLD file
if there are no matching names.

Following discussions in https://lists.osgeo.org/pipermail/mapserver-dev/2024-February/017105.html, the following approaches have been implemented:

  • For SLD files with multiple NamedLayers, if the Name matches the layer names then this will be used.
  • If there are no matches then by default the first named layer will be used. A new SLD_USE_FIRST_NAMEDLAYER metadata item is created to apply this logic by default. It can be set in the Mapfile to only apply NamedLayer if there is a match in the SLD file using METADATA "SLD_USE_FIRST_NAMEDLAYER" "false" (or any other value than true).
    Suggestions on any better name for this metadata key appreciated.
  • I have also added in support for SLD to be used in attributes, similar to the OGR styling support added in https://mapserver.org/development/rfc/ms-rfc-61.html. This had a few issues to resolve in msApplySldLayerToMapLayer as msSLDApplySLD had up until now only worked on layers that were closed. As the SLD is applied on a per-feature basis while looping through features logic was added to avoid re-opening or closing the layer. As suggested by @szekerest msLayerGetFeatureStyle was updated to call msSLDApplySLD. Applying SLD is done at a layer level, so there will be cases such as filters that won't work correctly here - but the basics of styling with SLD will work. Many MapServer services such as GetStyles will already fail if attempting to use on layers using per-feature styling, so the SLD support will work in the same way.

Implementation Details

I went on a few dead-ends in terms of where to apply the SLD to the layer. Initially I had this in the drawing functionality. However it became apparent the file would also need to be applied in other cases, such as in GetStyle or legend requests. This created much more complicated logic throughout the codebase. A simple approach was to apply the SLD styling when the Mapfile was parsed by adding logic to msApplyDefaultSubstitutions. This seems to fit as the SLD STYLEITEM is in effect replacing a CLASS with classes generated from the SLD. Cases such as overriding the styling with WMS SLD_BODY requests were then handled by default.

Initially I thoughy maybe returning the SLD file directly for WMS GetStyle requests could be an option. However users can request the SLD in different versions, so the SLD would need to be processed anyway - it was easier to convert to CLASS/STYLEs and then generate the SLD back from this as needed. There maybe some differences in the input/output SLD as part of this roundtrip.

Note the Mapfile will be modified by applying the SLD file (as with other items such as WFS filters, SLD form URLs etc.) so Mapfiles cannot be reused in subsequent requests (see similar discussions at #4018).

If an SLD file is missing the logs will have the error msSLDApplyFromFile(): Unable to access file. Invalid SLD filename: "sld/data/missing.sld".
If there is a CLASS in the LAYER then this will be used as a fallback for styling, otherwise the layer will produce an empty output.

Along with a test suite for the new functionality a couple of missing tests have been added for existing functionality to ensure it is not broken by changes in this pull request:

  • A test for msautotest/misc/ogrpen.map added to check the output of feature styling
  • wms_get_map_mark_symbol_sld_legend to test GetLegendGraphic request when SLD_BODY applied

Reviews and comments appreciated! Docs will follow once this pull request is finalised.

src/mapdraw.c Outdated Show resolved Hide resolved
src/mapfile.c Outdated Show resolved Hide resolved
src/mapfile.c Outdated Show resolved Hide resolved
src/maplayer.c Show resolved Hide resolved
src/mapogcsld.cpp Outdated Show resolved Hide resolved
src/mapogcsld.cpp Outdated Show resolved Hide resolved
src/mapogcsld.cpp Outdated Show resolved Hide resolved
src/mapogcsld.cpp Outdated Show resolved Hide resolved
src/mapogcsld.cpp Outdated Show resolved Hide resolved
src/mapogcsld.h Outdated Show resolved Hide resolved
src/mapdraw.c Outdated Show resolved Hide resolved
src/maplayer.c Outdated Show resolved Hide resolved
src/maplayer.c Outdated Show resolved Hide resolved
commit 4753416746033d8a8f39673f796e436034cfc245
Merge: 7173853 2525432
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Mar 3 17:24:20 2024 +0100

    Use SLD_USE_FIRST_NAMEDLAYER and check for true value
    Add logging and move loading of SLD
    Add full test suite

commit 7173853
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Mar 3 17:24:05 2024 +0100

    Ensure break out of loop

commit 2525432
Merge: c2b9a50 ab65f9c
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Mar 3 16:40:54 2024 +0100

    Merge branch 'rfc-138' of https://github.com/geographika/mapserver into rfc-138

commit c2b9a50
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Mar 3 16:40:41 2024 +0100

    Add and fix test suite

commit ab65f9c
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Mar 3 16:39:24 2024 +0100

    Fix return value

commit 42939de
Merge: 29aff9e 42ba716
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Mar 3 16:26:06 2024 +0100

    Merge branch 'rfc-138' of https://github.com/geographika/mapserver into rfc-138

commit 29aff9e
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Mar 3 16:24:44 2024 +0100

    Apply first style by default and load SLD when map is parsed

commit 42ba716
Author: sethg <sethg@geographika.co.uk>
Date:   Sat Mar 2 16:48:54 2024 +0100

    Fix line endings

commit d404d7f
Author: sethg <sethg@geographika.co.uk>
Date:   Sat Mar 2 15:53:21 2024 +0100

    Add test suite for RFC

commit 4b75db3
Author: sethg <sethg@geographika.co.uk>
Date:   Sat Mar 2 14:41:33 2024 +0100

    Add legend image output

commit 3169216
Merge: ab52675 ef0db3d
Author: sethg <sethg@geographika.co.uk>
Date:   Sat Mar 2 09:32:26 2024 +0100

    Revert comments

commit ab52675
Author: sethg <sethg@geographika.co.uk>
Date:   Sat Mar 2 09:18:07 2024 +0100

    Ignore tests for now

commit 4bde2b9
Author: sethg <sethg@geographika.co.uk>
Date:   Fri Mar 1 13:55:38 2024 +0100

    Add missing header

commit f4b35ae
Author: sethg <sethg@geographika.co.uk>
Date:   Fri Mar 1 13:42:24 2024 +0100

    Apply SLD from STYLEITEM field

commit f52354f
Author: sethg <sethg@geographika.co.uk>
Date:   Sat Feb 24 15:39:31 2024 +0100

    Add draft tests

commit e31d8c0
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Feb 18 21:51:11 2024 +0100

    Remove check

commit a88acaf
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Feb 18 21:36:29 2024 +0100

    Fix return values

commit 75f4152
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Feb 18 21:26:41 2024 +0100

    Initial commit

commit ef0db3d
Author: sethg <sethg@geographika.co.uk>
Date:   Fri Mar 1 13:55:38 2024 +0100

    Add missing header

commit 8bf365d
Author: sethg <sethg@geographika.co.uk>
Date:   Fri Mar 1 13:42:24 2024 +0100

    Apply SLD from STYLEITEM field

commit aa24b4d
Author: sethg <sethg@geographika.co.uk>
Date:   Sat Feb 24 15:39:31 2024 +0100

    Add draft tests

commit 02dfdbd
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Feb 18 21:51:11 2024 +0100

    Remove check

commit f3610f2
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Feb 18 21:36:29 2024 +0100

    Fix return values

commit dcb57de
Author: sethg <sethg@geographika.co.uk>
Date:   Sun Feb 18 21:26:41 2024 +0100

    Initial commit
@geographika
Copy link
Member Author

Final commits include:

  • add a test to apply SLD generated in QGIS to a raster file.
    See https://docs.geoserver.org/main/en/user/styling/qgis/index.html for exporting SLD from QGIS. However by default QGIS export <UserLayer>. This needs to be changed to <NamedLayer> to work with MapServer.

  • The style attribute SLD test was updated to use the new METADATA key "SLD_USE_FIRST_NAMEDLAYER" "true" and one of the SLD strings had a NamedLayer Name set to a different value than its layer. The METADATA key
    allows the SLD to be applied to the feature even though the names don't match.

  • I also added the NULL check when opening the SLD file, suggested in the code review, to the original function I copied.

All good to go from my side (apart from checking why Appveyor isn't starting) and ready for the 8.2 release.

Thanks very much for the review @rouault!

@geographika
Copy link
Member Author

Looks like the new raster test found a memory leak in an existing function msSLDParseRasterSymbolizer: https://github.com/MapServer/MapServer/actions/runs/8164584864/job/22320124941?pr=7034#step:3:9222

Will investigate further.

@rouault
Copy link
Contributor

rouault commented Mar 6, 2024

@geographika I've pushed an extra commit in your branch for a better fix for the memleak. Your initial change could cause a potential use-after-free when classitem was not null and different from "[pixel]".

@pvgenuchten
Copy link

Nice work Seth and Even, looking forward to test

@geographika geographika merged commit ae5e9b2 into MapServer:main Mar 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants