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

Centerline GEOMTRANSFORM Support #6417

Merged
merged 43 commits into from
Oct 14, 2021
Merged

Centerline GEOMTRANSFORM Support #6417

merged 43 commits into from
Oct 14, 2021

Conversation

sdlime
Copy link
Member

@sdlime sdlime commented Oct 5, 2021

Rework of the original #5854 pull request. Cleaned up a few pieces of the implementation and renamed skeletonize to centerline - it's a much more accurate name.

@sdlime sdlime marked this pull request as draft October 5, 2021 02:53
@LarsSchy
Copy link

LarsSchy commented Oct 5, 2021

I am missing mapgraph.c and mapgraph.h when I tried to build.

@jmckenna jmckenna added this to the 8.0 Release milestone Oct 5, 2021
@LarsSchy
Copy link

LarsSchy commented Oct 5, 2021

I am very happy with the results now!

(smoothsia(centerline([shape]), 3, 1, 'angle'))

test_250K_TT32

@jratike80
Copy link

The repeated labels next. Not drawing the first label at the bottom of a bay is a good idea, but what if the requested image bbox cuts the lake at the middle? How could you define that the first label on the centerline would be OK then?

@sdlime
Copy link
Member Author

sdlime commented Oct 8, 2021

mapgraph.cpp could benefit for more C++ification, but that's an implementation detail that can be changed later

Agree'd on the C++ification. Does "add suggestion to batch" essentially build a single pull request? I've not noticed that option before.

@rouault
Copy link
Contributor

rouault commented Oct 8, 2021

Does "add suggestion to batch" essentially build a single pull request?

a single commit added at the end of that PR. That's the preferred solution to avoid stressing too much the CI

Co-authored-by: Even Rouault <even.rouault@spatialys.com>
mapgraph.cpp Outdated Show resolved Hide resolved
@sdlime
Copy link
Member Author

sdlime commented Oct 13, 2021

Any objections to merging? I think it's in pretty good shape... --Steve

@jmckenna
Copy link
Member

Working great here. +1

@LarsSchy
Copy link

It a fantastic new feature! I just love it. To bad that the chaining (smoothsia(centerline([shape]))) doesn't work. It worked up until 8 days ago. It is strange since other chaining like (simplifypt(centerline([shape]... is working. I have even tried to look at the source code, but that parsing is a little too complicated for me.

@jmckenna
Copy link
Member

@LarsSchy, true, smoothsia no longer works for me either (alone, not even chained).

@sdlime
Copy link
Member Author

sdlime commented Oct 13, 2021

That's so weird and I experience the same thing. I imagine it's very fixable given a little time.

@geographika
Copy link
Member

Good news is everything compiles fine and an image is generated.

If I add a label to the the following layer definition then I can get an image back:

    LAYER
        TYPE LINE
        STATUS DEFAULT
        NAME "lakes_label"
        GEOMTRANSFORM (densify([shape], 5))
        CONNECTIONTYPE OGR
        CONNECTION "/data/natural_earth_vector.gpkg/packages/natural_earth_vector.gpkg"
        DATA "ne_10m_lakes"
        CLASS
          LABEL
            TEXT "hello"
            ANGLE FOLLOW
            SIZE 12
          END
        END
    END

image

However if I change to the following:

GEOMTRANSFORM (centerline(densify([shape], 100)))

Then I always get back the following error no matter what parameter I set for densify:

msGEOSCenterline(): GEOS library error. Centerline generation failed, try densifying the shapes. <br>

Am I configuring the layer incorrectly?

@sdlime
Copy link
Member Author

sdlime commented Oct 13, 2021

Am I configuring the layer incorrectly?

Configuration looks ok at first glance. That error message occurs when there are no edges added to the directed graph computed from the Voronoi diagram. I've only seen that happen in cases with super simple geometries like a square or triangle with vertices only at the corners. The algorithm removes Densifying solves that problem. What do you get if you just do?

GEOMTRANSFORM (centerline([shape]))

Any chance you could share your test case?

--Steve

@geographika
Copy link
Member

@sdlime - same error with that change (plus an additional parsing error which seems to be thrown after the first error):

msDrawMap(): Image handling error. Failed to draw layer named 'lakes_label'. <br>
msGeomTransformShape(): Expression parser error. Failed to process shape expression: centerline([shape]) <br>
yyparse(): Expression parser error. Executing centerline failed. <br>
msGEOSCenterline(): GEOS library error. Centerline generation failed, try densifying the shapes. <br>

I used the following data for the test case:

http://naciscdn.org/naturalearth/packages/natural_earth_vector.gpkg.zip

Command:

shp2img -m lakes.map -o /temp/test.png

Map file:

MAP
    SIZE 400 400
    EXTENT -68.7202 53.0750 -67.7202 54.0750

    LAYER
        TYPE LINE
        STATUS DEFAULT
        NAME "lakes_label"
        GEOMTRANSFORM (centerline([shape]))
        CONNECTIONTYPE OGR
        CONNECTION "natural_earth_vector.gpkg/packages/natural_earth_vector.gpkg"
        DATA "ne_10m_lakes"
        CLASS
          LABEL
            TEXT "hello" # "[name]"
            ANGLE FOLLOW
            SIZE 12
          END
        END
    END

    LAYER
        TYPE LINE
        GEOMTRANSFORM (outer([shape]))
        STATUS DEFAULT
        NAME "lakes"
        CONNECTIONTYPE OGR
        CONNECTION "natural_earth_vector.gpkg/packages/natural_earth_vector.gpkg"
        DATA "ne_10m_lakes"
        CLASS
          STYLE
            COLOR 0 0 255
            WIDTH 2
          END
        END
    END
END

@LarsSchy
Copy link

My experiments are working fine now with: (smoothsia(centerline([shape]))) !

I looked at the natural earth data from Seth. When I looked at the clipped data in QGIS, I can see several very small triangles. Some are in the data set from the beginning and some are due to the clipping.

ogr2ogr natural_earth_vector_lakes10m_clip.gpkg -clipsrc -68.7202 53.075 -67.7202 54.075 natural_earth_vector.gpkg ne_10m_lakes

@geographika
Copy link
Member

I looked at the natural earth data from Seth. When I looked at the clipped data in QGIS, I can see several very small triangles. Some are in the data set from the beginning and some are due to the clipping.

Thanks @LarsSchy for the investigations. That corresponds with Steve's comment about simple features.

Maybe these errors should be logged and ignored so that centerlines can still be generated for other features in the layer?

@sdlime
Copy link
Member Author

sdlime commented Oct 14, 2021

My experiments are working fine now with: (smoothsia(centerline([shape]))) !

I looked at the natural earth data from Seth. When I looked at the clipped data in QGIS, I can see several very small triangles. Some are in the data set from the beginning and some are due to the clipping.

ogr2ogr natural_earth_vector_lakes10m_clip.gpkg -clipsrc -68.7202 53.075 -67.7202 54.075 natural_earth_vector.gpkg ne_10m_lakes

The problem with that natural earth data is the super small polygons, there's one in that viewport that has an area of .000001 the error message @geographika was seeing was related to that geometry. The error message is relevant, a solution is to densify, but you have to use such a small value that it becomes counter productive. I needed a value of .001 in the densify function to get things to complete. A better solution will be to filter out those small geometries ahead of time, so for example if I do:

FILTER (area([shape]) > 0.005)
GEOMTRANSFORM (centerline([shape]))

to the lakes_label layer then things work fine. Making sure that only shapes that are candidates for labeling are going to be processed is probably going to be a common step - it also is more performant. So I don't see a blocker here. There may be some opportunities for some optimization with simple geometries - perhaps an alternative way to generate a centerline without going through a Voronoi diagram-based process.

--Steve

@sdlime
Copy link
Member Author

sdlime commented Oct 14, 2021

Maybe these errors should be logged and ignored so that centerlines can still be generated for other features in the layer?

I think that's going to be difficult based on the way the parser works since there may be downstream processing (e.g. SmoothSIA) so there's no way to bail mid-expression evaluation with a "never mind" response. I don't know if that's possible or not.

@geographika
Copy link
Member

Thanks Steve for the explanation. I guess I chose the wrong dataset to test with!
A note in the docs seems fine at this stage. Maybe a hard-coded pre-filter could be used to skip these features in future based on area or vertices.

Is it correct that the new keywords from this feature to add to https://mapserver.org/mapfile/geomtransform.html are centerline, densify, outer, and inner ?

@sdlime
Copy link
Member Author

sdlime commented Oct 14, 2021

Thanks Steve for the explanation. I guess I chose the wrong dataset to test with! A note in the docs seems fine at this stage. Maybe a hard-coded pre-filter could be used to skip these features in future based on area or vertices.

If you didn't hit a case like that then someone else would have! I think there will be further enhancements necessary in this area as the feature gets used.

Is it correct that the new keywords from this feature to add to https://mapserver.org/mapfile/geomtransform.html are centerline, densify, outer, and inner ?

Yup, those are the new GEOMTRANSFORM functions. The first 3 are probably relevant for drawing labels and would typically be applied at the LAYER level because of the way the path following labels work. Outer and inner may also be useful at the STYLE level for rendering effects.

@sdlime sdlime merged commit a40170b into MapServer:main Oct 14, 2021
@sdlime sdlime deleted the centerline branch October 14, 2021 17:47
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.

6 participants