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

Support for importing non-area relations as MultiLineString #176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

talaj
Copy link
Contributor

@talaj talaj commented Oct 26, 2018

It can be convenient to be able to import non-area relations as MultiLineString geometries into linestring table.

This PR adds this capability. Only relations specified in relation_types will be imported. No relations are imported by default.

@talaj talaj force-pushed the multilinestring branch 2 times, most recently from e37bbcd to 736f4b9 Compare October 26, 2018 13:43
@olt
Copy link
Member

olt commented Nov 9, 2018

Hi. Thanks for your PR.

The timing is a bit unfortunate as I just made a larger refactoring. Parsing and some basic stuff is moved into a separate library (https://github.com/omniscale/go-osm). The changes are the go-osm branch (https://github.com/omniscale/imposm3/tree/go-osm).

Can you base your patch on this branch and see if you need to make any changes?

For tests: It's great that you included a new system test. But can your use short and commented dummy OSM files like https://github.com/omniscale/imposm3/blob/go-osm/test/route_relation.osm and https://github.com/omniscale/imposm3/blob/go-osm/test/route_relation.osc

@talaj talaj force-pushed the multilinestring branch 2 times, most recently from da3e4fb to 1b8bd62 Compare November 9, 2018 11:55
@talaj
Copy link
Contributor Author

talaj commented Nov 9, 2018

Hi @olt. I've rebased it on top of go-osm branch. There were just minor conflicts. The OSM files should be more comprehensible now.

@olt olt changed the base branch from master to go-osm November 9, 2018 12:27
"runtime"
)

func BuildMultiLinestring(rel *osm.Relation, srid int) (*geos.Geom, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this into geom.go? There is a multipolygon.go, but this handling is also much more complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

@@ -0,0 +1,43 @@
package geom

import (
Copy link
Member

Choose a reason for hiding this comment

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

Can you sort the import as in other packages (or use goimports instead of gofmt).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked with goimports.

"github.com/omniscale/imposm3/geom/geos"
)

func TestSimpleMultiLineString(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This can be moved into geom_test.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

@@ -0,0 +1,52 @@
<?xml version='1.0' encoding='UTF-8'?>
<osm version="1">
Copy link
Member

Choose a reason for hiding this comment

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

Much better. Thanks!

@talaj talaj force-pushed the multilinestring branch 2 times, most recently from 0f15bc0 to eab7319 Compare November 9, 2018 13:48
@komackaj
Copy link

komackaj commented Oct 9, 2020

@olt Any chance of this being merged? Would be super-useful for me.

@talaj talaj changed the base branch from go-osm to master November 4, 2020 12:40
@talaj
Copy link
Contributor Author

talaj commented Nov 4, 2020

I've rebased it on top of master and changed the base branch to master. Surprisingly, there was no conflict after those two years.

@farfromrefug
Copy link

@olt I am in the need for that PR. Any reason why it is left unmerged?

@olt
Copy link
Member

olt commented Aug 23, 2022

Sorry for the long silence.
Is my understanding correct, that this change is backward incompatible with older imports? (It will try to import multipolygons into linestring tables?)

Can someone who used this patch comment on that?

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.

4 participants