-
Notifications
You must be signed in to change notification settings - Fork 157
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
base: master
Are you sure you want to change the base?
Conversation
e37bbcd
to
736f4b9
Compare
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 |
da3e4fb
to
1b8bd62
Compare
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. |
1b8bd62
to
63535d5
Compare
63535d5
to
dfcc9b4
Compare
geom/multilinestring.go
Outdated
"runtime" | ||
) | ||
|
||
func BuildMultiLinestring(rel *osm.Relation, srid int) (*geos.Geom, error) { |
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.
Can you move this into geom.go? There is a multipolygon.go, but this handling is also much more complex.
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.
Moved.
geom/multilinestring.go
Outdated
@@ -0,0 +1,43 @@ | |||
package geom | |||
|
|||
import ( |
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.
Can you sort the import as in other packages (or use goimports instead of gofmt).
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.
Checked with goimports.
geom/multilinestring_test.go
Outdated
"github.com/omniscale/imposm3/geom/geos" | ||
) | ||
|
||
func TestSimpleMultiLineString(t *testing.T) { |
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.
This can be moved into geom_test.go.
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.
Moved.
@@ -0,0 +1,52 @@ | |||
<?xml version='1.0' encoding='UTF-8'?> | |||
<osm version="1"> |
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.
Much better. Thanks!
0f15bc0
to
eab7319
Compare
@olt Any chance of this being merged? Would be super-useful for me. |
eab7319
to
c532aab
Compare
I've rebased it on top of master and changed the base branch to master. Surprisingly, there was no conflict after those two years. |
c532aab
to
18a7fe9
Compare
@olt I am in the need for that PR. Any reason why it is left unmerged? |
Sorry for the long silence. Can someone who used this patch comment on that? |
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.