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

Polyline and Polygon issues at higher zoom levels with/wo hardware acceleration #725

Closed
spyhunter99 opened this Issue Oct 1, 2017 · 42 comments

Comments

Projects
None yet
3 participants
@spyhunter99
Collaborator

spyhunter99 commented Oct 1, 2017

Priority:
This is blocking issue for 6.0.0 release. If we can't get everything rendering at the higher zoom levels, we may have to set the max zoom to the level at which we can render the content at.

Goals:
Provide proper rendering of polylines and polygons at any zoom level supported by the map view/projection with and without hardware acceleration turned on (really on is the goal)

Current issues after the merge of #329 #723

  • With hardware acceleration off
    • polyline disappears at zoom 25
    • polygon border disappears at zoom levels > 22
    • polygon has strange rendering behavior when the map wraps n/s, e/w
  • hw acceleration on
    • polyline disappears at zoom 26
    • polygon disappears when zooming in, it seems to be linked to large polygons that are primarily all offscreen see #353. Small polygons are not effected by this. The solution is an accurate clipping mechanism that supports all these strange edge cases when stuff is within or outside of the view port, holes and all at poles

Gotcha's

  • Polygons are complex.
    • "holes" that are partially offscreen, fill colors.
    • polygons in which all points are off screen, however the view port is within the polygon
    • polygons in which all points are off screen, however the view port is partially over the polygon
    • polygons in which all points are off screen, however the view port is completely over the polygon
  • lines
    • 1 or more points are on screen others are off screen
    • All points are on screen
    • All points are off screen, however the viewport covers an area in between some of the points
    • Poll/dateline wrapping

Considerations

  • Performance is a big one

Directly related issues

Indirectly related issues

  • #595 scaling during animations of polyline

@spyhunter99 spyhunter99 added this to the v6.0.0 milestone Oct 1, 2017

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 1, 2017

Collaborator

Some remarks

We shouldn't have different codes for Polygon and Polyline. They should both use Path. A Polyline behaves as a Polygon except that there are no holes, no fill and no Path.close().

In my tests polygon borders even disappear at level 23 (when I draw a polygon at level 0)

The expected behavior of Polygons in zoom level 0 is not clear for me, because we potentially have several pixels that match the same GeoPoint.
Could we say that:

  • we want only one Polygon displayed
  • whenever the same Polygon could be displayed in different drawn worlds, we favor the one with the biggest displayed pixels coverage, and among them the one that is top left?
Collaborator

monsieurtanuki commented Oct 1, 2017

Some remarks

We shouldn't have different codes for Polygon and Polyline. They should both use Path. A Polyline behaves as a Polygon except that there are no holes, no fill and no Path.close().

In my tests polygon borders even disappear at level 23 (when I draw a polygon at level 0)

The expected behavior of Polygons in zoom level 0 is not clear for me, because we potentially have several pixels that match the same GeoPoint.
Could we say that:

  • we want only one Polygon displayed
  • whenever the same Polygon could be displayed in different drawn worlds, we favor the one with the biggest displayed pixels coverage, and among them the one that is top left?
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 1, 2017

Collaborator

Concur on both.

Collaborator

spyhunter99 commented Oct 1, 2017

Concur on both.

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 1, 2017

Collaborator

I managed to do something: Polygons ok on zoom 29.
20171001 polygon zoom0
20171001 polygon zoom29

The code needs to be cleaned and checked again.
Then, the zoom 0 issues.

Collaborator

monsieurtanuki commented Oct 1, 2017

I managed to do something: Polygons ok on zoom 29.
20171001 polygon zoom0
20171001 polygon zoom29

The code needs to be cleaned and checked again.
Then, the zoom 0 issues.

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 1, 2017

Collaborator

well that's awesome, also check with "holes"

Collaborator

spyhunter99 commented Oct 1, 2017

well that's awesome, also check with "holes"

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Oct 2, 2017

First part of #725 - Polyline and Polygon issues at higher zoom level…
…s with/wo hardware acceleration

With this fix, the `Polygon`s looks OK up to zoom 29.
What remains to be done:
* check with holes
* same work on `Polyline`
* low-zoom "best top-left version" of `Polygon` / `Polyline`

Regarding this delivery...

New classes:
* `SegmentIntersection`: tools in order to compute the intersection between two 2D segments through `public` method `intersection`
* `SegmentIntersectionTest`: unit tests on `SegmentIntersection`

Impacted classes:
* `PointL`: added overriden methods `toString` and `equals`; added methods `set` and `squareDistanceTo`
* `Polygon`: modified method `buildPathPortion` in order to use less overflow prone `PointL` (instead of `Point`), to consider `Path` as a list of 2D segments, and to clip those segments into a min/max values square; added methods `clip`, `isInClipArea`, `intersection`, `lineTo`
* `Projection`: added methods `getLongPixelsFromProjected`, `getLongPixelXFromMercator`, `getLongPixelYFromMercator` and `getLongPixelFromMercator` for overflow reasons; removed 2 unused imports
@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 2, 2017

Collaborator

I've just pushed my fix for Polygons in zoom level 29 (monsieurtanuki:feature/#725).

I fully understand what is expected about the low-zoom "best top-left version" of Polygon.

I even understand most of what is expected for Polylines, although there are some unexpected specificities like method addGreatCircle and click events.

What is not clear for me is to "check Polygon with holes".
I can't really figure out what a Polygon with holes stands for. Would it be something like a map of continental Europe (Polygon) without Switzerland (hole 1) and Hungary (hole 2)?
Currently if I run demo "More samples / Drawing / Draw a Polygon with holes on screen", the hole has the same border and the same background color as the Polygon.
My assumption would be that the map under the hole would be displayed without alpha Paint on top. Am I correct?

Collaborator

monsieurtanuki commented Oct 2, 2017

I've just pushed my fix for Polygons in zoom level 29 (monsieurtanuki:feature/#725).

I fully understand what is expected about the low-zoom "best top-left version" of Polygon.

I even understand most of what is expected for Polylines, although there are some unexpected specificities like method addGreatCircle and click events.

What is not clear for me is to "check Polygon with holes".
I can't really figure out what a Polygon with holes stands for. Would it be something like a map of continental Europe (Polygon) without Switzerland (hole 1) and Hungary (hole 2)?
Currently if I run demo "More samples / Drawing / Draw a Polygon with holes on screen", the hole has the same border and the same background color as the Polygon.
My assumption would be that the map under the hole would be displayed without alpha Paint on top. Am I correct?

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 3, 2017

Collaborator

Your description is accurate. The demo with holes allows you to draw a ploygon, remove part of its center using the knife. The hole is drawn without the fill color.

Collaborator

spyhunter99 commented Oct 3, 2017

Your description is accurate. The demo with holes allows you to draw a ploygon, remove part of its center using the knife. The hole is drawn without the fill color.

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 3, 2017

Collaborator

There's something wrong with the algorithm: incoherent behavior between Polygon and GeoPoint.

Let's say the Polygon is the USA, and the GeoPoint is New York City.

My screen displays 2x2 worlds (something like a 512x512 screen).

If the top left of my screen is on Siberia, the Polygon is displayed top left, and so is the GeoPoint. The GeoPoint (NYC) is displayed inside the Polygon (USA). Looks like a sensitive behavior.

But if I scroll the map so that Chicago is on x=0, the NYC GeoPoint will still be on top left, but the US Polygon will be displayed in the middle of the screen (x-wise). The GeoPoint and the Polygon will be disconnected. Looks like a bug to me.

First question: do we agree on the fact that the zoom 0 specific algorithm is flawed?

Second question: could there be a better algorithm that solve this USA/NYC combination?
I don't think so, unless we add some parameter to the draw Polygon feature (like offset or justify)

Third question: could we be more relaxed about the zoom 0 specific algorithm, as it's a very specific use case to display the whole world (and more) on a smartphone screen?
As I assume no algorithm can be perfect on that subject, I would recommend that we consider the first GeoPoint of a Polygon to be top left, with the following Polygon's GeoPoints' coordinates computed from this start.
Does that make sense?

Collaborator

monsieurtanuki commented Oct 3, 2017

There's something wrong with the algorithm: incoherent behavior between Polygon and GeoPoint.

Let's say the Polygon is the USA, and the GeoPoint is New York City.

My screen displays 2x2 worlds (something like a 512x512 screen).

If the top left of my screen is on Siberia, the Polygon is displayed top left, and so is the GeoPoint. The GeoPoint (NYC) is displayed inside the Polygon (USA). Looks like a sensitive behavior.

But if I scroll the map so that Chicago is on x=0, the NYC GeoPoint will still be on top left, but the US Polygon will be displayed in the middle of the screen (x-wise). The GeoPoint and the Polygon will be disconnected. Looks like a bug to me.

First question: do we agree on the fact that the zoom 0 specific algorithm is flawed?

Second question: could there be a better algorithm that solve this USA/NYC combination?
I don't think so, unless we add some parameter to the draw Polygon feature (like offset or justify)

Third question: could we be more relaxed about the zoom 0 specific algorithm, as it's a very specific use case to display the whole world (and more) on a smartphone screen?
As I assume no algorithm can be perfect on that subject, I would recommend that we consider the first GeoPoint of a Polygon to be top left, with the following Polygon's GeoPoints' coordinates computed from this start.
Does that make sense?

@MKergall

This comment has been minimized.

Show comment
Hide comment
@MKergall

MKergall Oct 3, 2017

Collaborator

"Third question: could we be more relaxed about the zoom 0 specific algorithm"
=> IMO, zoom 0 doesn't make sense. Repetition is ugly, useless and a nightmare to support.

What most common online maps services are doing? Here are my findings, translated in osmdroid Min zoom levels "equivalents":
Google Maps: 2
Bing Maps: 2
openstreetmap: 0, or -1 (I can get up to 6 repetitions)
mapquest : 1
Mappy: 4
Yandex: 1

=> our Min zoom level could be not less than 1.

"can we impose the first GeoPoint of a Polygon to be top left": I don't think such a constraint is acceptable.

Collaborator

MKergall commented Oct 3, 2017

"Third question: could we be more relaxed about the zoom 0 specific algorithm"
=> IMO, zoom 0 doesn't make sense. Repetition is ugly, useless and a nightmare to support.

What most common online maps services are doing? Here are my findings, translated in osmdroid Min zoom levels "equivalents":
Google Maps: 2
Bing Maps: 2
openstreetmap: 0, or -1 (I can get up to 6 repetitions)
mapquest : 1
Mappy: 4
Yandex: 1

=> our Min zoom level could be not less than 1.

"can we impose the first GeoPoint of a Polygon to be top left": I don't think such a constraint is acceptable.

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 4, 2017

Collaborator

@MKergall I agree with you on world repetitions. More specifically, the S85 / N85 latitude transition makes no sense at all.

Maybe #722 is the correct answer and should be the default mode (disclaimer: I just read the description).

"can we impose the first GeoPoint of a Polygon to be top left": I don't think such a constraint is acceptable.

Hey, I never said "impose"! ;)
To clarify my thoughts: I don't mean at all we should expect/demand the users' Polygon's first GeoPoint to be the top left one of the GeoPoint list. I just wondered if the algorithm in the lousy repeated world case could be:

  • the Polygon starts with a GeoPoint
  • this GeoPoint can be projected on many pixels on this repeated world map - and among them we choose the top left pixel (cf. NYC example above)
  • all the other Polygon GeoPoints are projected accordingly - among all the projected pixels we choose the one that is closest to the chosen projected pixel of the previous GeoPoint

Alternate version - we stick to what I suggested earlier:

whenever the same Polygon could be displayed in different drawn worlds, we favor the one with the biggest displayed pixels coverage, and among them the one that is top left

Knowing that anyway the USA Polygon vs NYC GeoPoint potential issue cannot be prevented.

Collaborator

monsieurtanuki commented Oct 4, 2017

@MKergall I agree with you on world repetitions. More specifically, the S85 / N85 latitude transition makes no sense at all.

Maybe #722 is the correct answer and should be the default mode (disclaimer: I just read the description).

"can we impose the first GeoPoint of a Polygon to be top left": I don't think such a constraint is acceptable.

Hey, I never said "impose"! ;)
To clarify my thoughts: I don't mean at all we should expect/demand the users' Polygon's first GeoPoint to be the top left one of the GeoPoint list. I just wondered if the algorithm in the lousy repeated world case could be:

  • the Polygon starts with a GeoPoint
  • this GeoPoint can be projected on many pixels on this repeated world map - and among them we choose the top left pixel (cf. NYC example above)
  • all the other Polygon GeoPoints are projected accordingly - among all the projected pixels we choose the one that is closest to the chosen projected pixel of the previous GeoPoint

Alternate version - we stick to what I suggested earlier:

whenever the same Polygon could be displayed in different drawn worlds, we favor the one with the biggest displayed pixels coverage, and among them the one that is top left

Knowing that anyway the USA Polygon vs NYC GeoPoint potential issue cannot be prevented.

@MKergall

This comment has been minimized.

Show comment
Hide comment
@MKergall

MKergall Oct 4, 2017

Collaborator

Understood :-)

Collaborator

MKergall commented Oct 4, 2017

Understood :-)

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 5, 2017

Collaborator

I'm fine with removing or disabling vertical wrapping, east west makes sense. For multiple worlds, I'm also fine with removing

Collaborator

spyhunter99 commented Oct 5, 2017

I'm fine with removing or disabling vertical wrapping, east west makes sense. For multiple worlds, I'm also fine with removing

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Oct 6, 2017

Last part of #725 - Polyline and Polygon issues at higher zoom levels…
… with/wo hardware acceleration :

* check with holes
* same work on `Polyline`
* low-zoom "best top-left version" of `Polygon` / `Polyline`

Regarding this delivery...

New classes:
* `Distance`: a tool class dedicated to the computation of 2D distances
* `DistanceTest`: a Unit Test class dedicated to `Distance`
* `LinearRing`: used to be an inner class in `Polygon` but needed to go out in order to be used by `Polyline` too; was enhanced too in order to match the new requirements and the new zoom level limit

Impacted classes:
* `PointL`: removed methods `squareDistanceTo` that are now more or less available in new dedicated class `Distance`
* `Polygon`: moved `LinearRing` code to the new eponymous class; handled a common offset for the main polygon and the holes
* `Polyline`: removed all the code that could now be handled by `LinearRing`
* `Projection`: added parameter `pCloser` to method `getLongPixelsFromProjected`
@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 6, 2017

Collaborator

Done and committed.

Collaborator

monsieurtanuki commented Oct 6, 2017

Done and committed.

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 6, 2017

Collaborator

test results:
hw acceleration off (current default):

  • polyline working perfectly
  • polygon (solids) kind of working, however the border is disappearing at max zoom -2, i had the whole polygon disappear at max zoom -9
  • polygon with holes, working, however the border is disappearing at max zoom -2

hw acceleration on:

  • polyline still disappears for long lengths
  • polygon super slow performance (crashed my genymotion vm)
  • polygon with holes, not yet tested
Collaborator

spyhunter99 commented Oct 6, 2017

test results:
hw acceleration off (current default):

  • polyline working perfectly
  • polygon (solids) kind of working, however the border is disappearing at max zoom -2, i had the whole polygon disappear at max zoom -9
  • polygon with holes, working, however the border is disappearing at max zoom -2

hw acceleration on:

  • polyline still disappears for long lengths
  • polygon super slow performance (crashed my genymotion vm)
  • polygon with holes, not yet tested
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 6, 2017

Collaborator

i'll add, that my test cases and generally very large polygons drawn at near min zoom, generally the size of the continental us

Collaborator

spyhunter99 commented Oct 6, 2017

i'll add, that my test cases and generally very large polygons drawn at near min zoom, generally the size of the continental us

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 7, 2017

Collaborator

There are 2 paths of optimization/bug fix.

The polygon (and all its consecutive segments) are clipped into a [min, min, max, max] area for two reasons:

  • fitting long pixels value (due to high zoom level) into acceptable int values (for Path). In that case, the min/max values of the clip area could simply be Integer.MIN_VALUE and Integer.MAX_VALUE
  • fixing the Android bug hardware acceleration bug on Path drawing. In that case, there's no explicit min/max value. I initially adjusted the values to Integer.MIN_VALUE / 8 and Integer.MAX_VALUE / 8 because it worked OK on my smartphone.

If we put high values for min/max:

  • we alter less the shape of the Polygon outside of the visible area. (the visible area will always be OK, unless min/max are smaller than the screen)
  • we have more points in the Path (less pixels will be truncated to min/max)
  • we are more likely to have hardware acceleration issues (but there's no explicit value)

If we put low values for min/max:

  • we alter more the shape of the Polygon outside of the visible area. I thought it could be an issue when zooming out: displaying a truncated version of the Polygon during the animation. But it doesn't seem to be the case, as the Polygon seems to be redrawn somehow (and therefore recomputed)
  • we get better performances because there are less points in the Path
  • we are less likely to have hardware acceleration issues.

I suggest the following values for LinearRing.mClipMax: 17000 (a lousy approximation of 2^8 * a screen size of 1000). And -17000 for min.

There's another optimization possibility but that's not the most important now.
The low level "best position best area top left" algorithm costs a lot, because we need to save the whole path into local PointL lists before displaying the Path, in order to compute the best pixel offset.
If we don't need this "best offset" (for instance because the zoom level is not that low), we can get rid of those local lists and directly populate the Path.

Collaborator

monsieurtanuki commented Oct 7, 2017

There are 2 paths of optimization/bug fix.

The polygon (and all its consecutive segments) are clipped into a [min, min, max, max] area for two reasons:

  • fitting long pixels value (due to high zoom level) into acceptable int values (for Path). In that case, the min/max values of the clip area could simply be Integer.MIN_VALUE and Integer.MAX_VALUE
  • fixing the Android bug hardware acceleration bug on Path drawing. In that case, there's no explicit min/max value. I initially adjusted the values to Integer.MIN_VALUE / 8 and Integer.MAX_VALUE / 8 because it worked OK on my smartphone.

If we put high values for min/max:

  • we alter less the shape of the Polygon outside of the visible area. (the visible area will always be OK, unless min/max are smaller than the screen)
  • we have more points in the Path (less pixels will be truncated to min/max)
  • we are more likely to have hardware acceleration issues (but there's no explicit value)

If we put low values for min/max:

  • we alter more the shape of the Polygon outside of the visible area. I thought it could be an issue when zooming out: displaying a truncated version of the Polygon during the animation. But it doesn't seem to be the case, as the Polygon seems to be redrawn somehow (and therefore recomputed)
  • we get better performances because there are less points in the Path
  • we are less likely to have hardware acceleration issues.

I suggest the following values for LinearRing.mClipMax: 17000 (a lousy approximation of 2^8 * a screen size of 1000). And -17000 for min.

There's another optimization possibility but that's not the most important now.
The low level "best position best area top left" algorithm costs a lot, because we need to save the whole path into local PointL lists before displaying the Path, in order to compute the best pixel offset.
If we don't need this "best offset" (for instance because the zoom level is not that low), we can get rid of those local lists and directly populate the Path.

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 7, 2017

Collaborator

@spyhunter99 I guess you can re-run your tests with my latest commit - or just set LinearRing.mClipMax to 17000 and LinearRing.mClipMin to -mClipMax.
It should solve the issues (disappearing border and very slow display).

Collaborator

monsieurtanuki commented Oct 7, 2017

@spyhunter99 I guess you can re-run your tests with my latest commit - or just set LinearRing.mClipMax to 17000 and LinearRing.mClipMin to -mClipMax.
It should solve the issues (disappearing border and very slow display).

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 7, 2017

Collaborator

test results

hardware acceleration on

  • polyline disappearing
  • polygon, super slow zooming, panning

hardware acceleration off

  • polyline disappearing, i also saw one case where a fling caused the line to disappear on the same zoom level in which it was visible. it would reappear i panned to another world
  • polygon border disappearing fixed
Collaborator

spyhunter99 commented Oct 7, 2017

test results

hardware acceleration on

  • polyline disappearing
  • polygon, super slow zooming, panning

hardware acceleration off

  • polyline disappearing, i also saw one case where a fling caused the line to disappear on the same zoom level in which it was visible. it would reappear i panned to another world
  • polygon border disappearing fixed
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 7, 2017

Collaborator

i'm thinking we need some way to automate some of these test cases and to add test cases related to very small drawings as well as very large ones

Collaborator

spyhunter99 commented Oct 7, 2017

i'm thinking we need some way to automate some of these test cases and to add test cases related to very small drawings as well as very large ones

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 7, 2017

Collaborator

I need you to give me all the data you use for your tests: Polygons with all their GeoPoints (most important), map center and zoom levels.
With that I can build a new test Activity and try to reproduce the bugs you encounter.
That's OK for visual tests.

But I don't really know how we could say programmatically that there's an issue. Perhaps by comparing some canvas pixel color before and after we add a Polygon.

Collaborator

monsieurtanuki commented Oct 7, 2017

I need you to give me all the data you use for your tests: Polygons with all their GeoPoints (most important), map center and zoom levels.
With that I can build a new test Activity and try to reproduce the bugs you encounter.
That's OK for visual tests.

But I don't really know how we could say programmatically that there's an issue. Perhaps by comparing some canvas pixel color before and after we add a Polygon.

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 7, 2017

Collaborator
Collaborator

spyhunter99 commented Oct 7, 2017

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 7, 2017

Collaborator

and that's just it, there's now way to programmatically tell if something has rendered or not, or is visible on screen or not, aside from checking pixel colors. Bummer

Collaborator

spyhunter99 commented Oct 7, 2017

and that's just it, there's now way to programmatically tell if something has rendered or not, or is visible on screen or not, aside from checking pixel colors. Bummer

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 7, 2017

Collaborator

https://youtu.be/-TKE2tDm7KU polygon with hardware acceleration test
https://youtu.be/Jc6tm07iQb0 polygon with software acceleration test
https://youtu.be/qB_IQ_FfnKY hardware acceleration line test
https://youtu.be/oB5Uf0KtcIc software acceleration, line test
https://youtu.be/ki5W26lynrU fling issue with software accerlation

Collaborator

spyhunter99 commented Oct 7, 2017

https://youtu.be/-TKE2tDm7KU polygon with hardware acceleration test
https://youtu.be/Jc6tm07iQb0 polygon with software acceleration test
https://youtu.be/qB_IQ_FfnKY hardware acceleration line test
https://youtu.be/oB5Uf0KtcIc software acceleration, line test
https://youtu.be/ki5W26lynrU fling issue with software accerlation

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 8, 2017

Collaborator

@spyhunter99 Thank you for your videos.
I focused on https://youtu.be/qB_IQ_FfnKY, which is a demo of the following bug: "when hardware acceleration is on, the polyline disappear beyond approximately zoom + 4".

I ran many experiments. For instance, this shows a path:

path.moveTo(-1554, 1410);
path.lineTo(-326, 400);
path.lineTo(11, 341);
path.lineTo(308, 321);
path.lineTo(565, 459);
path.lineTo(763, 816);
path.lineTo(2512, 1229);

But that one doesn't show anything, though the only difference is the last x lineTo parameter, with a difference of 1:

path.moveTo(-1554, 1410);
path.lineTo(-326, 400);
path.lineTo(11, 341);
path.lineTo(308, 321);
path.lineTo(565, 459);
path.lineTo(763, 816);
path.lineTo(2513, 1229);

It looks like there's a drawing issue if the path covered area goes beyond a bit above 4x1024x1024 pixels.
The solution would therefore be to lower the clip area down to 2000x2000.
I set LinearRing.mClipMin to -600 and LinearRing.mClipMax to 1400.
And now the polylines and polygons are always displayed on my smartphone, with and without hardware acceleration.

Is that 4Mpixels limit relevant on all devices? I don't know.

@spyhunter99 Could you please test again: basically my assumption is that there should not path not drawing issue again.

As I stated previously, if the only problem left is speed there are still margins of optimization, as we compute tons of variables only for the low zoom level possible offset.

In "https://youtu.be/ki5W26lynrU fling issue with software acceleration" I'm not sure I understand the issue. If the problem is that the polyline is sometimes jumping when the map is panning, that's exactly what the business rule is: we offset the polyline where it covers the maximum visible area and possibly top left. Is that what bothers you? Or is it the slow speed of the fling?

Collaborator

monsieurtanuki commented Oct 8, 2017

@spyhunter99 Thank you for your videos.
I focused on https://youtu.be/qB_IQ_FfnKY, which is a demo of the following bug: "when hardware acceleration is on, the polyline disappear beyond approximately zoom + 4".

I ran many experiments. For instance, this shows a path:

path.moveTo(-1554, 1410);
path.lineTo(-326, 400);
path.lineTo(11, 341);
path.lineTo(308, 321);
path.lineTo(565, 459);
path.lineTo(763, 816);
path.lineTo(2512, 1229);

But that one doesn't show anything, though the only difference is the last x lineTo parameter, with a difference of 1:

path.moveTo(-1554, 1410);
path.lineTo(-326, 400);
path.lineTo(11, 341);
path.lineTo(308, 321);
path.lineTo(565, 459);
path.lineTo(763, 816);
path.lineTo(2513, 1229);

It looks like there's a drawing issue if the path covered area goes beyond a bit above 4x1024x1024 pixels.
The solution would therefore be to lower the clip area down to 2000x2000.
I set LinearRing.mClipMin to -600 and LinearRing.mClipMax to 1400.
And now the polylines and polygons are always displayed on my smartphone, with and without hardware acceleration.

Is that 4Mpixels limit relevant on all devices? I don't know.

@spyhunter99 Could you please test again: basically my assumption is that there should not path not drawing issue again.

As I stated previously, if the only problem left is speed there are still margins of optimization, as we compute tons of variables only for the low zoom level possible offset.

In "https://youtu.be/ki5W26lynrU fling issue with software acceleration" I'm not sure I understand the issue. If the problem is that the polyline is sometimes jumping when the map is panning, that's exactly what the business rule is: we offset the polyline where it covers the maximum visible area and possibly top left. Is that what bothers you? Or is it the slow speed of the fling?

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 8, 2017

Collaborator

Great job on fixing the issue. i'll run some more tests.

The panning issue. While slowly panning, i was noticing that the map was unexpectedly jumping around. Like the map was jumping, not the line.

Collaborator

spyhunter99 commented Oct 8, 2017

Great job on fixing the issue. i'll run some more tests.

The panning issue. While slowly panning, i was noticing that the map was unexpectedly jumping around. Like the map was jumping, not the line.

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 9, 2017

Collaborator

There are side effects to such a small clip area, for instance in low zoom level when searching for the best location: if I add a world size to my polyline's pixels, I may go beyond the clip area.
Still things to fix...

Collaborator

monsieurtanuki commented Oct 9, 2017

There are side effects to such a small clip area, for instance in low zoom level when searching for the best location: if I add a world size to my polyline's pixels, I may go beyond the clip area.
Still things to fix...

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 9, 2017

Collaborator

lines
hw https://youtu.be/AtQQde1f8N4
sw https://youtu.be/_eF7r3ss5CY

lines appear to randomly redraw and strange angles seen on both with and without hardware accel. The fling issue is also still apparent.

Collaborator

spyhunter99 commented Oct 9, 2017

lines
hw https://youtu.be/AtQQde1f8N4
sw https://youtu.be/_eF7r3ss5CY

lines appear to randomly redraw and strange angles seen on both with and without hardware accel. The fling issue is also still apparent.

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Oct 9, 2017

Final (?) fix for #725
Impacted classes:
* `LinearRing`: used `List<RectL>` for segments instead of `List<PointL>` for both segments start and end points; added method `applyOffset`; changed `getBestOffset` so that it focuses on the smallest distance to the screen center rather than on the biggest common area with the screen rect; removed `getCommonArea`
* `RectL`: added constructor `RectL(RectL other)` and method `set(RectL other)`
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 9, 2017

Collaborator

@monsieurtanuki just pulled, compile failed

C:\projects\osmdroid\osmdroid-android\src\main\java\org\osmdroid\views\overlay\LinearRing.java:218: error: no suitable constructor found for RectL(RectL)
                                pSegments.add(new RectL(currentSegment));
                                              ^
    constructor RectL.RectL() is not applicable
      (actual and formal argument lists differ in length)
    constructor RectL.RectL(long,long,long,long) is not applicable
      (actual and formal argument lists differ in length)
Note: Some input files use or override a deprecated API.
Collaborator

spyhunter99 commented Oct 9, 2017

@monsieurtanuki just pulled, compile failed

C:\projects\osmdroid\osmdroid-android\src\main\java\org\osmdroid\views\overlay\LinearRing.java:218: error: no suitable constructor found for RectL(RectL)
                                pSegments.add(new RectL(currentSegment));
                                              ^
    constructor RectL.RectL() is not applicable
      (actual and formal argument lists differ in length)
    constructor RectL.RectL(long,long,long,long) is not applicable
      (actual and formal argument lists differ in length)
Note: Some input files use or override a deprecated API.
@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 9, 2017

Collaborator

That's what I meant: this comes from a collision between the clip area being very small AND the computation of the offset for very low zoom level.
But that's the past: I just fixed it (I compute the offset AND THEN I clip the area, not the other way around).

I've just committed a very nice version that should solve everything.

Except for one thing: the algorithm for very low zoom levels is to put the Polygon or Polyline where their centers are the closest to the screen (map) center.
The previous business rule was to put them top left with the best possible covered area.
As everybody here despises the very low zoom levels, I assume we can agree on this new decent business rule without too much friction. Can't we?

[about the RectL compilation issue, I just forgot to commit/push RectL. Added it now]

Collaborator

monsieurtanuki commented Oct 9, 2017

That's what I meant: this comes from a collision between the clip area being very small AND the computation of the offset for very low zoom level.
But that's the past: I just fixed it (I compute the offset AND THEN I clip the area, not the other way around).

I've just committed a very nice version that should solve everything.

Except for one thing: the algorithm for very low zoom levels is to put the Polygon or Polyline where their centers are the closest to the screen (map) center.
The previous business rule was to put them top left with the best possible covered area.
As everybody here despises the very low zoom levels, I assume we can agree on this new decent business rule without too much friction. Can't we?

[about the RectL compilation issue, I just forgot to commit/push RectL. Added it now]

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 9, 2017

Collaborator

definitely better! business rule change seems fine, as long as it renders

ran into an issue with polygons on software acceleration... https://youtu.be/lhlFkA4h8D4 it's changing shapes with zooming/panning,

issue is also present with hardware acceleration https://youtu.be/1zCjNb2bTdk

Collaborator

spyhunter99 commented Oct 9, 2017

definitely better! business rule change seems fine, as long as it renders

ran into an issue with polygons on software acceleration... https://youtu.be/lhlFkA4h8D4 it's changing shapes with zooming/panning,

issue is also present with hardware acceleration https://youtu.be/1zCjNb2bTdk

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 9, 2017

Collaborator

Now it should be OK (just committed/pushed again).
At last.
More robust segment clipping.

For the record I created a funny shaped Polygon in order to get the same unexpected behaviors as in your youtube videos:

final Polygon polygon = new Polygon();
polygon.setFillColor(Color.argb(75, 0,255,0));
final List<GeoPoint> points = new ArrayList<>();
points.add(new GeoPoint(0., 0));
points.add(new GeoPoint(-10., 10));
points.add(new GeoPoint(-5., 15));
points.add(new GeoPoint(-7., 5));
points.add(new GeoPoint(-12., 20));
points.add(new GeoPoint(-50., 0));
polygon.setPoints(points);
mMapView.getOverlayManager().add(polygon);

Could be useful later for tests.

Collaborator

monsieurtanuki commented Oct 9, 2017

Now it should be OK (just committed/pushed again).
At last.
More robust segment clipping.

For the record I created a funny shaped Polygon in order to get the same unexpected behaviors as in your youtube videos:

final Polygon polygon = new Polygon();
polygon.setFillColor(Color.argb(75, 0,255,0));
final List<GeoPoint> points = new ArrayList<>();
points.add(new GeoPoint(0., 0));
points.add(new GeoPoint(-10., 10));
points.add(new GeoPoint(-5., 15));
points.add(new GeoPoint(-7., 5));
points.add(new GeoPoint(-12., 20));
points.add(new GeoPoint(-50., 0));
polygon.setPoints(points);
mMapView.getOverlayManager().add(polygon);

Could be useful later for tests.

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 10, 2017

Collaborator

I'm positive you now appreciate the complexities of drawing map data. Welcome to the club. I'm also very happy that you were able to resolve this.

I think this is the first time I've ever seen polygons work with hardware acceleration without clipping the shape before adding it to the map. bravo! I also tested with map rotation and everything seemed ok.

i did see one strange issue when i drew a polygon on screen and after it plotted, it moved. I'm not sure if it's a projection issue or point reducer or what... anyhow i'll merge it as is and we can move on.

@MKergall might finally be happy about the hardware acceleration support

Collaborator

spyhunter99 commented Oct 10, 2017

I'm positive you now appreciate the complexities of drawing map data. Welcome to the club. I'm also very happy that you were able to resolve this.

I think this is the first time I've ever seen polygons work with hardware acceleration without clipping the shape before adding it to the map. bravo! I also tested with map rotation and everything seemed ok.

i did see one strange issue when i drew a polygon on screen and after it plotted, it moved. I'm not sure if it's a projection issue or point reducer or what... anyhow i'll merge it as is and we can move on.

@MKergall might finally be happy about the hardware acceleration support

spyhunter99 added a commit that referenced this issue Oct 10, 2017

Feature/#725 (#729)
* feature/#329: store and restore the actual map center (instead of scroll) and the map orientation

This is the first (slightly unrelated) step.
Why should we use the map center instead of the scroll? Because when tilting the smartphone portrait-landscape-wise, the map center is kept only modulo the difference between `MapView`'s width and height.

Impactes classes:
* `OpenStreetMapConstants`: deprecated scroll preference tags, added latitude, longitude and orientation preference tags
* `MapView`: added the concept of `IGeoPoint initCenter`, took it into account in method `myOnLayout`, added a no-force-redraw version of method `setMapOrientation`
* `StarterMapFragment`: deprecated the use of scroll preferences tags, introduced the use of new preferences' tags (latitude, longitude, orientation), gently refactored in order to avoid Android Studio's nagging

* bug/#683: mMapView.zoomToBoundingBox is not calculating the required zoom level correctly

This commit:
* fixes the calculation of the zoom that matches a bounding box
* adds Unit Tests for `TileSystem`
* makes a more explicit demo

Impact on existing classes:
* `BoundingBox`: created method `getCenterWithDateLine` that takes into consideration the date line; deprecated `getCenter`
* `MapView`: created method `zoomToBoundingBox` with an additional `borderSizeInPixels` parameter; modified previous method `zoomToBoundingBox` in order to use the new `zoomToBoundingBox` with 0 as border size; fixed the bounding box zoom calculation that is now testable and moved to `TileSystem.getBoundingBoxZoom`
* `TileSystem`: created methods `getBoundingBoxZoom`, `getLongitudeZoom`, `getLatitudeZoom`, `getX01FromLongitude`and `getY01FromLatitude`; modified `LatLongToPixelXYMapSize` in order to take into account the new "XY01" methods; gently refactored javadoc version `6.0.0` to `5.6.6`
* `SampleZoomToBounding`: made a more explicit test (which is accessed in "More Samples / Events / Zoom to Bounding Box" demo)

New class:
* `TileSystemTest`: created in order to test methods `getY01FromLatitude`, `getX01FromLongitude`, `LatLongToPixelXYMapSize` and `getBoundingBoxZoom`

* Fixed and enhanced a unit test

Impact on existing classes:
* OpenStreetMapViewTest: added random iterations; set zoom before center for decent results; added a +-1 tolerance because of my benevolence towards rounding; wrote a relevant test tag; removed commented and deprecated code

* Fixed and enhanced unit tests

Impact on existing classes:
* Bug445Caching: made the test less resource dependent by limiting max zoom level to 16 (cf. https://operations.osmfoundation.org/policies/tiles/: "[tiles at zoom levels 17 and higher] are generally not available (cached) on the server in advance, and have to be rendered specifically for those requests, putting an unjustified burden on the available resources"); gently refactoring javadoc from `6.0` to `5.6.6`
* TileSystemTest: made the XY01 test stronger by checking [0,1]

* #329 Zoom level is now up to 29

New files:
* 30 `.png` files corresponding to the tiles of the same place on the 30 zoom levels from 0 to 29. I had no way to compute real tiles for high zoom levels, therefore I created the "Abstract" source of tiles with simple colored .png where the zoom level is displayed.

New classes:
* `PointL`: like a `Point`, but with long instead of int
* `ProjectionTest`: a unit test class for `Projection`
* `RectL`: like a `Rect`, but with long instead of int
* `SampleVeryHighZoomLevel`: a demo dedicated to high zoom levels, available in "More Samples / Tileproviders / Offline abstract tiles for zoom levels 0 to 29"

Biggest impacts on existing classes:
* `MapView`: used `PointL` and `RectL` types for Projected data; removed `initCenter`; added `GeoPoint mCenter`, `long mMapScrollX` and `mMapScrollY` and their setters/getters; overridden `scrollBy`; changed `scrollTo`; added `getMapScale`
* `Projection`: created a new constructor without any reference to `MapView`; created `getOffspring` in order to compute a `Projection` from another (e.g. for `MinimapOverlay`); renamed methods for clarity; created _many_ methods
* org.osmdroid.util.TileSystem: renamed methods for clarity; used `PointL` and `RectL` types for Mercator data; created _many_ methods

Other impacted classes:
* `CacheManager`: removed useless code that would have required refactoring
* `GeometryMath`: refactored `DEG2RAD` / `RAD2DEG`; used `MyMath.floorToInt` instead of `(int)` for better handling of negative values; used simpler syntax on `Rect` for easier unit testing purposes
* `GeoPoint`: created method `distanceToAsDouble` to go beyond the meter; used a distance calculation algorithm more precise for small distances; increased precision from float to double
* `GeoPointTest`: create methods `test_distanceTo_itself`, `test_distanceTo_Equator`, `test_distanceTo_Equator_Smaller`, `test_distanceTo_Parallels`, `getCleanLongitudeDiff`, `getRandomLongitude`, `getRandomLatitude`; removed less relevant methods `test_distanceTo_zero`, `test_distanceTo_one`, test_distanceTo_one_degree`
* `MapController`: changed animation methods according to the new scroll behavior
* `MapTileProviderBase`: used `PointL` and `RectL` types for Mercator data
* `Marker`: refactored the use of methods from `Projection` to `MapView`
* `MathConstants`: increased precision from float to double
* `MinimapOverlay`: relied more on inherited methods than on specific code
* `MyLocationNewOverlay`: used `PointL` type for Projected data; using `double` instead of `int` zoom level; using new method `Projection.getPixelsFromProjected`
* `MyMath`: created new methods `flootToLong` and `flootToInt` in order to fix a counter intuitive Java behavior
* `OpenStreetMapViewTest`: unrelated light refactoring
* `OsmBitmapShader`: used `PointL` type for Mercator data
* `PathOverlay`: used `PointL` and `RectL` types for Projected data
* `PathProjection`: used `PointL` type for Mercator data
* `Polygon`: used `PointL` type for Projected data
* `Polyline`: used `PointL` type for Projected data
* `SampleAnimateTo`: changed slightly for testing reasons
* `SampleFactory`: added new class `SampleVeryHighZoomLevel`
* `SampleZoomToBounding`: unrelated light refactoring
* `ScaleBarOverlay`: used more precise new method `GeoPoint.distanceToAsDouble`; handled double distances in method `scaleBarLengthText`; added helper method `getScaleString`; refactored the use of methods from `Projection` to `MapView`
* `StarterMapFragment`: unrelated bug fix *** setInitCenter
* `TileLooper`: used `RectL` type for Mercator data; slightly refactored
* `TilesOverlay`: used `PointL` and `RectL` types for Mercator data; refactored `onTileReadyToDraw`; created `protected` methods `setCanvasRect`, `getCanvasRect`, `setProjection` and `getProjection`
* `microsoft.mappoint.TileSystem`: used 64 instead of 32 as a limit parameter in `setTileSize`; introduced the notions of `primaryKeyMaxZoomLevel` and `projectionZoomLevel`
* `TileSystemTest`: added `testGetLatitudeFromY01`, `testLatitude`, `testGetLongitudeFromX01`, `testLongitude`, `checkLatitude` and `checkLongitude`; removed `testLatLongToPixelXYMapSize`

* Javadoc gentle fix

* Javadoc gentle fix

* Javadoc gentle fix

* Javadoc gentle fix

* Javadoc gentle fix

* Javadoc gentle fix

* Javadoc gentle fix

* Gentle conflict fix

* Gentle conflict fix

* Gentle conflict fix

* Impacts:
* `SampleFactory`: added a reference to new demo `SampleVeryHighZoomLevel`
* `TileSystem`: switched to `double` zoom level versions for methods `MapScale` and `GroundResolution`
* `TileSystemMathTest`: moved to `TileSystemTest` methods `test_MapSize`, `test_groundResolution` and `test_groundMapScale`
* `TileSystemTest`: moved from `TileSystemTest` methods `test_MapSize`, `test_groundResolution` and `test_groundMapScale`; improved their handling of high zoom levels

* Fixed `Polygon` issue as in https://www.youtube.com/watch?v=S4Pf2u9WSyQ

Impacts in `Polygon` class:
* `mOriginalPoints` is now an array of `double`, no more `intE6` (but it's not related to the bug fixing)
* new method `setCloserPoint`: the assumption is that 2 consecutive points should be as close as possible, therefore we add or subtract "the size of the world" if necessary

* First part of #725 - Polyline and Polygon issues at higher zoom levels with/wo hardware acceleration

With this fix, the `Polygon`s looks OK up to zoom 29.
What remains to be done:
* check with holes
* same work on `Polyline`
* low-zoom "best top-left version" of `Polygon` / `Polyline`

Regarding this delivery...

New classes:
* `SegmentIntersection`: tools in order to compute the intersection between two 2D segments through `public` method `intersection`
* `SegmentIntersectionTest`: unit tests on `SegmentIntersection`

Impacted classes:
* `PointL`: added overriden methods `toString` and `equals`; added methods `set` and `squareDistanceTo`
* `Polygon`: modified method `buildPathPortion` in order to use less overflow prone `PointL` (instead of `Point`), to consider `Path` as a list of 2D segments, and to clip those segments into a min/max values square; added methods `clip`, `isInClipArea`, `intersection`, `lineTo`
* `Projection`: added methods `getLongPixelsFromProjected`, `getLongPixelXFromMercator`, `getLongPixelYFromMercator` and `getLongPixelFromMercator` for overflow reasons; removed 2 unused imports

* Last part of #725 - Polyline and Polygon issues at higher zoom levels with/wo hardware acceleration :
* check with holes
* same work on `Polyline`
* low-zoom "best top-left version" of `Polygon` / `Polyline`

Regarding this delivery...

New classes:
* `Distance`: a tool class dedicated to the computation of 2D distances
* `DistanceTest`: a Unit Test class dedicated to `Distance`
* `LinearRing`: used to be an inner class in `Polygon` but needed to go out in order to be used by `Polyline` too; was enhanced too in order to match the new requirements and the new zoom level limit

Impacted classes:
* `PointL`: removed methods `squareDistanceTo` that are now more or less available in new dedicated class `Distance`
* `Polygon`: moved `LinearRing` code to the new eponymous class; handled a common offset for the main polygon and the holes
* `Polyline`: removed all the code that could now be handled by `LinearRing`
* `Projection`: added parameter `pCloser` to method `getLongPixelsFromProjected`

* Lower clip area threshold values:
* mClipMax is now 17000 (instead of Integer.MAX_VALUE / 8)
* mClipMin is now -mClipMax (instead of Integer.MIN_VALUE / 8)

* Modified `Bug445Caching` in order to avoid random Travis crash on "queue size":
* added `ensureCapacity` in order to actually cache all tiles
* added methods `getMaxTileExpected`, `getMinNumberExpected`, `getMaxNumberExpected`

* Lowered the clip area size in `LinearRing` in order to fix hardware acceleration related path not displaying bugs:
* `mClipMax` = 1400
* `mClipMin` = -600

* Final (?) fix for #725

Impacted classes:
* `LinearRing`: used `List<RectL>` for segments instead of `List<PointL>` for both segments start and end points; added method `applyOffset`; changed `getBestOffset` so that it focuses on the smallest distance to the screen center rather than on the biggest common area with the screen rect; removed `getCommonArea`
* `RectL`: added constructor `RectL(RectL other)` and method `set(RectL other)`

* (forgotten file of previous commit/push)

* Fix as we cannot use only `path.close()`. More robust segment clipping.

New class:
* `SegmentClipper`: a tool to clip segments
* `SegmentClipperTest`: a Unit Test class dedicated to `SegmentClipper`

Impacted classes:
* `LinearRing`: now has a constructor with a `Path` as a parameter; now implements `SegmentClipper.SegmentClippable`; moved clipping code to new class `SegmentClipper`; added parameter `boolean pClosePath` to method `getSegmentsFromProjected` as we cannot just use `path.close()` with the clipping process; remove parameter `boolean pClosePath` from method `getPathFromSegments` for the same reason ;)
* `PointL`: added constructor `PointL(PointL other)`
* `Polygon`: impacted the fact that `LinearRing`'s constructor now demands a `Path`
* `Polyline`: impacted the fact that `LinearRing`'s constructor now demands a `Path`
* `RectL`: added overridden methods `equals` and `hashCode`
@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 24, 2017

Collaborator

(continued from #742)
@Maradox The LinearRing clip area is merely a rectangle just beyond the screen rectangle.
Every Polygon will be truncated at display time so that it won't go beyond those clip boundaries. It sounds pointless (because we cannot see beyond the screen rect) but this way we avoid an Android Polygon display bug.

For instance, if the screen rectangle is [0, 0, 600, 800], it makes sense to have a clip area like [-600, -600, 1400, 1400], because

  • the clip area encloses the screen rect
  • the screen rect is vaguely centered on the clip area
  • the 2000 pixel spans look experimentally OK on my smartphone

With a 1080×1920 resolution a clip area of [-600, -600, 1400, 1400] does not make any sense, as we would truncate the bottom-right part of the Polygons. The minimum rectangle would be [0, 0, 1080, 1920] (the screen rect), and [-500,-500, 1500, 2500] if we apply a 500 delta.

It looks like we need to compute mClipMin and mClipMax for each smartphone.
What I can tell you is that on my smartphone [-600, -600, 1400, 1400] is the ideal clip area rectangle (if bigger we get the Android bug).
But I have no way to know the correct size of the clip area for other smartphones :(

Collaborator

monsieurtanuki commented Oct 24, 2017

(continued from #742)
@Maradox The LinearRing clip area is merely a rectangle just beyond the screen rectangle.
Every Polygon will be truncated at display time so that it won't go beyond those clip boundaries. It sounds pointless (because we cannot see beyond the screen rect) but this way we avoid an Android Polygon display bug.

For instance, if the screen rectangle is [0, 0, 600, 800], it makes sense to have a clip area like [-600, -600, 1400, 1400], because

  • the clip area encloses the screen rect
  • the screen rect is vaguely centered on the clip area
  • the 2000 pixel spans look experimentally OK on my smartphone

With a 1080×1920 resolution a clip area of [-600, -600, 1400, 1400] does not make any sense, as we would truncate the bottom-right part of the Polygons. The minimum rectangle would be [0, 0, 1080, 1920] (the screen rect), and [-500,-500, 1500, 2500] if we apply a 500 delta.

It looks like we need to compute mClipMin and mClipMax for each smartphone.
What I can tell you is that on my smartphone [-600, -600, 1400, 1400] is the ideal clip area rectangle (if bigger we get the Android bug).
But I have no way to know the correct size of the clip area for other smartphones :(

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Oct 24, 2017

Collaborator
Collaborator

spyhunter99 commented Oct 24, 2017

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 25, 2017

Collaborator

I'm currently creating a demo Activity dedicated to those hardware acceleration issues.

Collaborator

monsieurtanuki commented Oct 25, 2017

I'm currently creating a demo Activity dedicated to those hardware acceleration issues.

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Oct 26, 2017

Collaborator

@spyhunter99 I don't think that supporting hardware acceleration on MapView makes sense.

There are unsupported features that we use, including Canvas.drawPath (still not implemented on Android 23). That means we will get potentially random rendering on different devices.

Possible ways to use hardware acceleration:

  1. test, fine tune and write specific code for all devices. Yuck!
  2. wait until Android supports drawPath
  3. warn users - "if you're lucky it's safe to use, if you're not turn it off" (current implementation)
  4. never use it - always turning it off for MapView
  5. redevelop all the MapView/Overlays drawing, in order to implement our own Path rendering or to avoid the use of Matrix

My suggestion is to turn hardware acceleration off. Or let the users turn it on if they want, but ignore any comment about the rendering when the hardware acceleration is on.
What do you guys think about it?

PS: Icing on the cake - my maps are slower with the hardware acceleration turn on :)

Collaborator

monsieurtanuki commented Oct 26, 2017

@spyhunter99 I don't think that supporting hardware acceleration on MapView makes sense.

There are unsupported features that we use, including Canvas.drawPath (still not implemented on Android 23). That means we will get potentially random rendering on different devices.

Possible ways to use hardware acceleration:

  1. test, fine tune and write specific code for all devices. Yuck!
  2. wait until Android supports drawPath
  3. warn users - "if you're lucky it's safe to use, if you're not turn it off" (current implementation)
  4. never use it - always turning it off for MapView
  5. redevelop all the MapView/Overlays drawing, in order to implement our own Path rendering or to avoid the use of Matrix

My suggestion is to turn hardware acceleration off. Or let the users turn it on if they want, but ignore any comment about the rendering when the hardware acceleration is on.
What do you guys think about it?

PS: Icing on the cake - my maps are slower with the hardware acceleration turn on :)

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Nov 3, 2017

Collaborator

bummer. alright i'll revert the commit which enables hardware acceleration on by default. Canvas.drawPath is implemented in android but has some scaling issues? I'm not sure i understand what that link is saying and the side effects a user of osmdroid may encounter with hardware acceleration on

Collaborator

spyhunter99 commented Nov 3, 2017

bummer. alright i'll revert the commit which enables hardware acceleration on by default. Canvas.drawPath is implemented in android but has some scaling issues? I'm not sure i understand what that link is saying and the side effects a user of osmdroid may encounter with hardware acceleration on

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Nov 3, 2017

Collaborator

My understanding is that in the context of hardware acceleration turned on:

  • Android does not support Canvas.drawPath (that we use when drawing Polygons and Polylines) during Canvas scaling (that we use in the Matrix part of Projection).
  • as a consequence, display results cannot be trusted to be relevant

Yesterday I had this idea that we may be able to remove the scaling part of the Matrix. Because we are able to draw scaled tiles and add points to a Polygon without a Matrix.
Would that mean that Canvas.drawPath would then work correctly with the hardware acceleration turned on? That should be checked first before starting the developments.

Collaborator

monsieurtanuki commented Nov 3, 2017

My understanding is that in the context of hardware acceleration turned on:

  • Android does not support Canvas.drawPath (that we use when drawing Polygons and Polylines) during Canvas scaling (that we use in the Matrix part of Projection).
  • as a consequence, display results cannot be trusted to be relevant

Yesterday I had this idea that we may be able to remove the scaling part of the Matrix. Because we are able to draw scaled tiles and add points to a Polygon without a Matrix.
Would that mean that Canvas.drawPath would then work correctly with the hardware acceleration turned on? That should be checked first before starting the developments.

@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Nov 14, 2017

Collaborator

I think I now have a partial answer. Not relative to hardware acceleration limitations: I think that should be put specifically in a distinct issue (that would deal with avoiding the Android unsupported operations mentioned above)

My main focus here is the problem experienced by @Maradox, which I would sum up by: "there are constant min/max values in LinearRing defining the clip area, and those values are too small for a 1080x1920 screen".

I think I managed to solve this issue: the clip area min/max are not constants anymore, but depend on the MapView characteristics (width, height, scale and orientation).
The results look OK so far, and I think I even managed to create a very small (but big enough) clip area, which should give better performances and potentially small enough for hardware acceleration (assuming Android's issues come from the Path area).

Still testing. About to PR.

Collaborator

monsieurtanuki commented Nov 14, 2017

I think I now have a partial answer. Not relative to hardware acceleration limitations: I think that should be put specifically in a distinct issue (that would deal with avoiding the Android unsupported operations mentioned above)

My main focus here is the problem experienced by @Maradox, which I would sum up by: "there are constant min/max values in LinearRing defining the clip area, and those values are too small for a 1080x1920 screen".

I think I managed to solve this issue: the clip area min/max are not constants anymore, but depend on the MapView characteristics (width, height, scale and orientation).
The results look OK so far, and I think I even managed to create a very small (but big enough) clip area, which should give better performances and potentially small enough for hardware acceleration (assuming Android's issues come from the Path area).

Still testing. About to PR.

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Nov 14, 2017

feature/#725 - the `LinearRing` clip area limits are not constants an…
…ymore, but depend on the current `MapView`

Impacted classes:
* `LinearRing`: removed clip area limits constants; created methods `setClipArea` in order to set the most relevant clip area according to the current `MapView`
* `Polygon`: used new method `LinearRing.setClipArea(MapView)`
* `Polyline`: used new method `LinearRing.setClipArea(MapView)`
@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Nov 14, 2017

Collaborator

(just PR'ed)

Collaborator

monsieurtanuki commented Nov 14, 2017

(just PR'ed)

@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Nov 15, 2017

Collaborator

sweet! does this value refresh when rotating the device?

Collaborator

spyhunter99 commented Nov 15, 2017

sweet! does this value refresh when rotating the device?

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Nov 15, 2017

feature/#725: Fixed the clip area size
Impacted class:
* `LinearRing`: used the size of the circle which contains the `MapView`'s 4 corners
@monsieurtanuki

This comment has been minimized.

Show comment
Hide comment
@monsieurtanuki

monsieurtanuki Nov 15, 2017

Collaborator

Even more often, because it depends on the current pinch scale and map orientation (I mean angle, not portrait/landscape).

Collaborator

monsieurtanuki commented Nov 15, 2017

Even more often, because it depends on the current pinch scale and map orientation (I mean angle, not portrait/landscape).

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Nov 16, 2017

feature/#725: Fixed a side effect
Impacted classes:
* `LinearRing`: gently cleaned the code in methods `lineTo` and `getSegmentsFromProjected`
* `SegmentClipper`: created method `getClosestCorner` in order to fix the algorithm for the case there's no intersection between the clip area and the segment to clip
* `SegmentClipperTest`: added tests related to the current fix

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Nov 16, 2017

feature/#725: Fixed a side effect
(just forgot those 2 files, which should not change a lot)

spyhunter99 added a commit that referenced this issue Nov 18, 2017

feature/#725 - computing the `LinearRing` clip area limits (#774)
* feature/#725 - the `LinearRing` clip area limits are not constants anymore, but depend on the current `MapView`

Impacted classes:
* `LinearRing`: removed clip area limits constants; created methods `setClipArea` in order to set the most relevant clip area according to the current `MapView`
* `Polygon`: used new method `LinearRing.setClipArea(MapView)`
* `Polyline`: used new method `LinearRing.setClipArea(MapView)`

* feature/#725: Fixed the clip area size

Impacted class:
* `LinearRing`: used the size of the circle which contains the `MapView`'s 4 corners

* feature/#725: Fixed a side effect

Impacted classes:
* `LinearRing`: gently cleaned the code in methods `lineTo` and `getSegmentsFromProjected`
* `SegmentClipper`: created method `getClosestCorner` in order to fix the algorithm for the case there's no intersection between the clip area and the segment to clip
* `SegmentClipperTest`: added tests related to the current fix

* feature/#725: Fixed a side effect

(just forgot those 2 files, which should not change a lot)
@spyhunter99

This comment has been minimized.

Show comment
Hide comment
@spyhunter99

spyhunter99 Nov 18, 2017

Collaborator

awesome thank you everyone for the excellent contributions. they are greatly appreciated!

Collaborator

spyhunter99 commented Nov 18, 2017

awesome thank you everyone for the excellent contributions. they are greatly appreciated!

monsieurtanuki added a commit to monsieurtanuki/osmdroid that referenced this issue Nov 20, 2017

feature/#725: Optimized execution performances
New method `SegmentClipper.set` to be used instead of frequent object creations

Impacted classes:
* `LinearRing`: used new method `SegmentClipper.set`
* `SegmentClipper`: added method `set` in order to optimize the execution performances by reusing the same object
* `SegmentClipperTest`: used new method `SegmentClipper.set`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment