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 custom non-crs proj transforms with new proj api again #4309

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

Conversation

josch
Copy link
Contributor

@josch josch commented Apr 15, 2022

No description provided.

@josch josch force-pushed the support-non-crs-input-again branch 2 times, most recently from a00817a to e9dc236 Compare April 15, 2022 05:44
@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #4309 (10801d4) into master (1ba1278) will decrease coverage by 0.03%.
The diff coverage is 55.40%.

@@            Coverage Diff             @@
##           master    #4309      +/-   ##
==========================================
- Coverage   75.04%   75.00%   -0.04%     
==========================================
  Files         511      511              
  Lines       36546    36610      +64     
==========================================
+ Hits        27425    27461      +36     
- Misses       9121     9149      +28     
Impacted Files Coverage Δ
src/proj_transform.cpp 74.61% <55.40%> (-4.54%) ⬇️
plugins/input/sqlite/sqlite_featureset.cpp 89.06% <0.00%> (-1.57%) ⬇️
src/text/itemizer.cpp 87.61% <0.00%> (-0.96%) ⬇️
src/png_reader.cpp 83.67% <0.00%> (+0.68%) ⬆️
src/svg/output/process_symbolizers.cpp 100.00% <0.00%> (+2.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ba1278...10801d4. Read the comment docs.

@josch josch marked this pull request as draft April 15, 2022 06:39
@josch
Copy link
Contributor Author

josch commented Apr 16, 2022

I marked this pull request as "Draft" for two reasons:

  1. unit tests are missing (and the decrease in coverage) -- please advice where you think I should put a good test for this alternative code path
  2. my tracks are not yet rendered correctly -- more on that now

So the basic I idea of this pull request to allow non-crs transforms already works well. Here is a rendering of the polygons for a tinshift transform:

out_debug

And here is a mapnik rendering with the map warped accordingly:

out

The problem is, the gpx track doesn't get warped as well but isn't showing up at all. I'm using the python mapnik bindings with the commits from the proj6 branch and am running it like this:

    tinshift = mapnik.Projection("+proj=pipeline +step +proj=webmerc +step +proj=tinshift +file=/tmp/tinshift.json")
    m = mapnik.Map(600, int(600*height/width))
    mapnik.load_map_from_string(m, open("mapnik.xml").read(), False, "/usr/share/openstreetmap-carto-common")
    gpxstyle = mapnik.Style()
    gpxrule = mapnik.Rule()
    gpxsym = mapnik.LineSymbolizer()
    gpxsym.stroke = mapnik.Color('blue')
    gpxsym.stroke_width = 5
    gpxsym.stroke_opacity = 0.5
    gpxrule.symbols.append(gpxsym)
    gpxstyle.rules.append(gpxrule)
    m.append_style('GPXStyle', gpxstyle)
    gpxlayer = mapnik.Layer('GPXLayer')
    gpxlayer.datasource = mapnik.Ogr(file = sys.argv[1], layer = 'tracks')
    gpxlayer.styles.append('GPXStyle')
    m.layers.append(gpxlayer)
    m.aspect_fix_mode = mapnik.aspect_fix_mode.GROW_BBOX
    m.zoom_to_box(mapnik.Box2d(mapnik.Coord(0, 0), mapnik.Coord(width, height)))
    m.srs = tinshift.params()
    surface = cairo.SVGSurface("out.svg", 600, int(600*height/width))
    mapnik.render(m, surface)
    surface.finish()

The same code renders the track just fine if I replace my +proj=pipeline with webmerc (see my tinshift debug rendering above -- the track draws just fine in blue). That's why I think the culprit is the non-webmerc projection. I'm also getting the following errors on the terminal when running above code:

webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
webmerc: Invalid latitude
ERROR 1: IllegalArgumentException: Points of LinearRing do not form a closed linestring
ERROR 1: IllegalArgumentException: Points of LinearRing do not form a closed linestring

Those errors go away if I comment out the line m.layers.append(gpxlayer). That's why I think that there is something missing in the parts that are involved in rendering the tracks. Can you give me some pointers?

@josch josch force-pushed the support-non-crs-input-again branch 2 times, most recently from 7583a83 to 73c1ab1 Compare April 18, 2022 06:26
@josch josch force-pushed the support-non-crs-input-again branch from 73c1ab1 to 10801d4 Compare April 18, 2022 09:41
@mathisloge
Copy link
Collaborator

@josch you can better view coverage here : https://app.codecov.io/gh/mapnik/mapnik/compare/4309/diff
as the codecov reporting in github is a bit buggy..

@josch
Copy link
Contributor Author

josch commented Apr 18, 2022

@josch you can better view coverage here : https://app.codecov.io/gh/mapnik/mapnik/compare/4309/diff
as the codecov reporting in github is a bit buggy..

Thanks, I think I now added a test case that correctly executes the new code path.

The only thing missing is the problem with correctly transforming my gpx layer. Any idea what could be wrong?

@josch josch marked this pull request as ready for review April 18, 2022 10:52
@mathisloge
Copy link
Collaborator

I can't really answer this, but maybe @artemp could.

@josch josch changed the title wip: support custom non-crs proj transforms again support custom non-crs proj transforms with new proj api again Apr 26, 2022
@josch
Copy link
Contributor Author

josch commented Jun 1, 2022

Hi, I wanted to send another friendly ping. Any update on this? Thanks!

@artemp
Copy link
Member

artemp commented Jun 1, 2022

Hi, I wanted to send another friendly ping. Any update on this? Thanks!

Hi @josch, sorry I didn't have a chance to check your PR, yet. I made a reminder for Monday. Thanks for your patience.

@artemp
Copy link
Member

artemp commented Jun 6, 2022

@josch - PR looks good, thanks. Could you share your GPX file and tinshift.json ?

@josch
Copy link
Contributor Author

josch commented Jun 6, 2022

@josch - PR looks good, thanks. Could you share your GPX file and tinshift.json ?

Sure, here you go: mapniktinshift.zip

@artemp
Copy link
Member

artemp commented Jun 7, 2022

@josch I finally had a chance to test your tinshift pipeline. I can replicate this issue with the following minimal Python script:

import mapnik
tinshift = mapnik.Projection("+proj=pipeline +step +proj=webmerc +step +proj=tinshift +file=/tmp/tinshift.json")
wgs84 = mapnik.Projection("epsg:4326")
tr=mapnik.ProjTransform(wgs84, tinshift)
tr.forward(mapnik.Coord(9.974552, 49.789942))
webmerc: Invalid latitude
Coord(inf,inf)

(NOTE: I had to disable adding +type=crs !!)

diff --git a/src/proj_transform.cpp b/src/proj_transform.cpp
index fa141cf30..a54fe1a20 100644
--- a/src/proj_transform.cpp
+++ b/src/proj_transform.cpp
@@ -100,7 +100,7 @@ std::string pj_add_type_crs_if_needed(const std::string &str) {
     if ((str.rfind("proj=", 0) == 0 || str.rfind("+proj=", 0) == 0 ||
          str.rfind("+init=", 0) == 0 || str.rfind("+title=", 0) == 0) &&
         str.find("type=crs") == std::string::npos) {
-        ret += " +type=crs";
+        //ret += " +type=crs";
     }
     return ret;
 }

Without the patch above ^ I was getting invalid PROJ string error

Omitting webmerc from pipeline e.g

tinshift = mapnik.Projection("+proj=pipeline +step +proj=tinshift +file=/home/artem/projects/mapnik/bugs/4309/tinshift.json")
tr=mapnik.ProjTransform(wgs84, tinshift)
tr.forward(mapnik.Coord(9.974552, 49.789942))
Coord(6505465.205830257,286994.6673597563)

works as in it produces some output. I don't really know much about pipeline framework, perhaps you can ask proj developers if your original definition is valid? If 'yes' then something is not right on mapnik side and needs more investigations/fixes. HTH

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.

None yet

3 participants