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

[v2.1.0] Fixes For Polar Projection #1295

Merged
merged 9 commits into from
Jul 20, 2022

Conversation

JosefWN
Copy link
Contributor

@JosefWN JosefWN commented Jun 30, 2022

Hi, I live in this crazy world where northWest is not upperLeft and Distance.offset(..., 180) is a movement in both x and y 🙃

This PR:

  • Adds a getOffset for convenience
  • Removes redundant pos.multiplyBy(map.getZoomScale(map.zoom, map.zoom)) (scale by 1 is meaningless)
  • Fixes issues with polar projection breaking ImageLayerWidget and CircleLayerWidget

Not entirely sure about this snippet though, why are we adding the offsets in _fillOffsets() twice if i > 0? Typo?

offsets.add(offset);
if (i > 0) {
  offsets.add(offset);
}

@JosefWN JosefWN force-pushed the polar-proj-fixes branch from 74de587 to 9f1f77a Compare June 30, 2022 08:26
Copy link
Contributor

@ibrierley ibrierley left a comment

Choose a reason for hiding this comment

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

Thanks for the work. Is there an example that could be included that highlights the problem for polar projection, may be handy for the future.

I think I'd like a slight renaming from getOffset to something a bit more obvious...i.e an offset from what ? Maybe something like getOffsetFromPixelOrigin or getOffsetFromOrigin ? Something along those lines anyway...may just make some of the other bits of code more obvious about what's being retrieved.

@JosefWN
Copy link
Contributor Author

JosefWN commented Jun 30, 2022

Hi, sure, you mean an example in the readme or in this issue?

@ibrierley
Copy link
Contributor

I think maybe in the examples folder, I'm not entirely clear what aspect was broken before yet, I was just thinking maybe it would be handy to have another page to test. That way if anyone introduces any further changes, we can make sure it doesn't break that example.

@JosefWN
Copy link
Contributor Author

JosefWN commented Jun 30, 2022

Here is a simple example:
Screen Shot 2022-06-30 at 13 23 41

The map is projected onto a plane and centered around the pole (North Pole in this case, could also be south). Note how the ship's track, marked in yellow, is heading past Svalbard toward the north pole, this is a diagonal movement roughly toward the upper left corner on my screen. Once I cross over the north pole I'm suddenly traveling south, even though I'm still traveling in the same direction on the screen, before the ship turns down toward Greenland.

In short: traveling north means traveling toward the center of the map. Traveling east means traveling in an arc parallel with the circular latitude lines in the grid.

@ibrierley
Copy link
Contributor

ibrierley commented Jun 30, 2022

Oddly that offsets duplication has always been there (in a slightly different form..)
Original

var i = 0;
          for (var point in polylineOpt.points) {
            var pos = map.project(point);
            pos = pos.multiplyBy(map.getZoomScale(map.zoom, map.zoom)) - map.getPixelOrigin();
            polylineOpt.offsets.add(new Offset(pos.x.toDouble(), pos.y.toDouble()));
            if (i > 0 && i < polylineOpt.points.length) {
              polylineOpt.offsets.add(new Offset(pos.x.toDouble(), pos.y.toDouble()));
            }
            i++;
          }

This one got my head scratching, but suddenly had a brainwave....

In the original, we used drawPoints using PointMode.lines which takes pairs of points and draws the lines. However, along the way, it was switched to Path()..addPolygon() which only needs points, not pairs of points. It just still happened to work, as no one noticed the duplicates.

So I think in polygon/line the extra if statement can be removed...

Now, it may be worth trying to compare performance drawPoints vs drawPath at some point if anyone has the will to live :D (wondering if drawPoints ends up with them cached in a gpu memory or anything).

Good spot on that btw, may be a slight performance tweak getting it right.

@ibrierley
Copy link
Contributor

I think I'd be more comfortable with an actual code example to test before/after. As well as helping to test it, it will help prevent any future breakages.

@JaffaKetchup
Copy link
Member

Agreed. Maybe a dedicated example page?

@JosefWN
Copy link
Contributor Author

JosefWN commented Jul 11, 2022

The "Custom CRS" example is actually using polar projection, but it gets a bit more interesting if you center on or near the pole. Polar projections are local projections, not global, intended specifically for areas around the poles.

As a first step I have added an "EPSG3413" example which is largely the same as the "Custom CRS" example, but instead centering the map on the North Pole which I believe is more common, and which makes the challenges of using this type of projection more pronounced.

I don't know how we should go about this though, do you want me to only add examples of overlay image and the circle layer? For all other layers supported by flutter_map we won't know if they actually work with polar coordinates until we try, but adding many layers in the example might clutter things?

In this PR I have included quick fixes for a couple of layers but I didn't intend to implement full support for polar projections, some layers will likely also be best kept apart, such as the grid plugin, since the polar grid is largely different in implementation from the lat/lon grid.

All in all I've found issues with:

  • The grid plugin (wouldn't call this a bug since it requires a fairly different implementation)
  • The scale bar (can likely be made universal, but for some reason I re-implemented it specifically for polar projections a while back)
  • The circle layer (made universal in this PR)
  • The overlay image layer (made universal in this PR)

Generally things work pretty well though, but I would be surprised if there weren't more issues in the layers I don't use, since I have a feeling I might be one of few using this type of projection.

@JaffaKetchup
Copy link
Member

@JosefWN I'm not really very knowledgeable about this, as you could probably guess. So I've probably misunderstood something, please correct me!

I don't know how we should go about this though, do you want me to only add examples of overlay image and the circle layer? For all other layers supported by flutter_map we won't know if they actually work with polar coordinates until we try, but adding many layers in the example might clutter things?

I think the dedicated page should have the overlay image and circle, and for the rest of the example pages, we can just manually test them with polar coords, we don't need to update them.

such as the grid plugin

In my opinion, we can make a seperate grid plugin for the polar regions somehow. On the other hand, the scale bar does need to be fixed in my opinion. I suspect @ibrierley may have some ideas on this topic.

At the same time, we should try to keep this non-breaking, unless essential.

@ibrierley Any more ideas?


PS. Might need looking at the conflicts from before near ce5d283#diff-11fa5da041e852102f076ee006e61d4e4aadfe0532312b20fcc8119414d1813c, now that #1294 has been merged.

@JosefWN
Copy link
Contributor Author

JosefWN commented Jul 11, 2022

Done, also fixed the dart fmt issue. Had a hard time finding a good overlay image example so I included a fairly small image (about 50 KB) as an asset, hope it's okay. The overlay image should be correctly positioned/oriented, and the circles should be of equal size on the screen.

Tiny nitpick, but in the "Custom CRS" example I think the coordinates are reversed for the chosen point:

// Define start center
proj4.Point point = proj4.Point(x: 65.05166470332148, y: -19.171744826394896);

I don't think it makes any difference the way that the variable is used, but semantically it would make sense if the x coordinate was the longitude and vice versa for latitude? Or the point could perhaps just be a LatLng (since that's all it's used to store)?

@JosefWN JosefWN force-pushed the polar-proj-fixes branch from 08d2ac0 to 2772595 Compare July 11, 2022 22:27
@JosefWN
Copy link
Contributor Author

JosefWN commented Jul 11, 2022

Quick test with/without my fixes.

Good:
Screen Shot 2022-07-12 at 00 15 14

Bad (3/5 circles showing but with incorrect radius, and no overlay image):
Screen Shot 2022-07-12 at 00 21 40

@JaffaKetchup
Copy link
Member

Thanks for fixing the format issues. It's fine to include an asset, you've managed to keep it small, which is a bonus!

It's entirely possible that variable was chosen to be a 'LatLng` for a reason, looking at the oldest version of the file ('latlong2' was available at the time it was written, so it was probably a conscious decision). So I'd probably just leave it as that type.
On the other hand, it would make sense for it to swap around. Just need to make sure it doesn't break anything, and wasn't chosen to be like that for a specific reason.

Will try to test this soon, but not sure what I'm looking for 100%, so it was be less in-depth than normal for a change of this size.

@JosefWN JosefWN force-pushed the polar-proj-fixes branch from f0f3bb4 to 18e3a22 Compare July 16, 2022 11:03
@JosefWN
Copy link
Contributor Author

JosefWN commented Jul 16, 2022

I have clarified the example now, also including brief explanations of the challenges and a link to this PR for more details. I have realized that the material online on polar projections isn't very pedagogical, so I'll try to be extra pedagogical in expanding on the example in this post as well.

For the overlay image it's pretty easy to see that the northern corners of the image are the ones closest to the North Pole (yellow dot), so assuming that the topLeft corner of the image is northWest will cause the code to produce an incorrect result.

The offset calculation in the circle layer assumed that an offset at bearing 180° (south) is a positive change only in the y coordinate, this is "vertically downwards" on the screen, since an image coordinate system is (0,0) at top left:

circle.realRadius = rpos.y - pos.y;

In a polar coordinate system the map is not oriented "north up" since north is a point on the map. If you had a compass that pointed toward the geographical North Pole and moved across the map (without rotating the map or yourself), the compass could show any value [0,360)° depending on where you were.

The orientation of the map is instead determined by the lon_0 parameter in the proj4 string, in our case the map is oriented lon_0=-45, so so the 45° W longitude line is vertically extending downward from the pole, other polar projections could have other orientations.

The direction "north" is the angle between your current position and the North Pole. The eastern direction vector is a 90° clockwise rotation of the northern vector, i.e. looking north you have east on your right. This means that east can also be any angle [0,360)°.

In other words, an offset south is very likely a change in both x and y, and assuming it is not will produce an incorrect (too small) circle radius, since y < hypot(x, y) unless x = 0. Making matters worse, when rpos.y < pos.y (when north is not up but down), the realRadius becomes negative, making the circle disappear altogether.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

OK, so I've just loaded up the example application, and it all makes a bit more sense now.

Unfortunately, your changes seem to have broken something:

Before After
Before After

Notice how one of the lines of the polygons is the wrong color. Also, you can't see the circular point on the purple polygon before, but you can now. It seems all the lines are thinner now? Not sure why this may have happened?

Sorry for the long response time, but at least I know what's going on now :)

@JaffaKetchup JaffaKetchup changed the title Fixes for polar projection [v2.1.0] Fixes For Polar Projection Jul 20, 2022
@JaffaKetchup JaffaKetchup self-assigned this Jul 20, 2022
@JosefWN JosefWN force-pushed the polar-proj-fixes branch 2 times, most recently from 361eff8 to bf226b3 Compare July 20, 2022 15:22
@JaffaKetchup JaffaKetchup self-requested a review July 20, 2022 15:23
@JosefWN
Copy link
Contributor Author

JosefWN commented Jul 20, 2022

Ah, sorry for missing that. It's caused by my removal of the offsets:

offsets.add(offset);
if(i > 0) {
  offsets.add(offset);
}

What's confusing is why _paintLine (solid line) is drawing a whole bunch of circles forming the line, it also paints the line itself:

void _paintLine(
    Canvas canvas, List<Offset> offsets, double radius, Paint paint) {
  canvas.drawPoints(PointMode.lines, [...offsets, offsets[0]], paint);
  for (final offset in offsets) {
    canvas.drawCircle(offset, radius, paint);
  }
}

Not really a bug but potentially a performance penalty, especially with complex polygons. I have aligned the functionality with the polyline layer now, making it possible to choose how you want your strokeCap and strokeJoin, and used early exits in both implementatins of _paintLine.

The default behavior should be in line with what it was before, but with improved performance and configurability.

@JosefWN JosefWN force-pushed the polar-proj-fixes branch from bf226b3 to dce34f6 Compare July 20, 2022 15:30
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

I see. It is strange that circles were being used, and I'd say that that was definitely a problem for performance, so thanks for spotting it.

All looks to be OK now, as far as I can see. Thanks for your contributions!

PS. Please try to avoid so many force-pushes if you can, it makes it harder to pull latest versions of your fork/branch :)

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.

3 participants