perf: pre-project polylines, and improve simplification & culling

[ignatz]

Project polylines at construction time in line with recent changes to polygons. This reduces the number of operations per frame. Also move simplifications into projected/planar space which helps to avoid distortions due to projections.

[ignatz]

FTR: I also took the liberty to restructure the culling code

[josxha]

lgtm, tested on windows

[JaffaKetchup]

Mostly LGTM, but this seems to have introduced an unusual bug.

In the example application, set the stroke width of the purple line to 100. Then pan the map, moving East. As expected, one of the purple line segements is culled too aggressively and disappears from the screen - this is expected behaviour, and the purpose of cullingMargin.
However, at the same time, the pink line gains the large 100 stroke width even though it was not changed.

2024-01-22.10-20-17.mp4

[ignatz]

Mostly LGTM, but this seems to have introduced an unusual bug.

There's a bug. We're not taking outlines into consideration (polygons probably have the same issue). That said, the bug predates this change and is observable at head. Its prominence might have changed (improved or worsened depending on projection, where on the planet and where on screen) since we're now culling in projected space rather than map space but that feels a bit potatoe potato.

[ignatz]

After reading the code a bit more carefully it's clear why it surfaced less before. There was this nonsensical cullingMargin parameter, which was gauged to degrees and now in projected space didn't provide the same level of loopdy-loop-coverup.

I took the liberty to remove it. Should be properly fixed now (famous last words).

[JaffaKetchup]

The bug was more about the pink line suddenly getting massive (see the video, was not present on master), but removing the culling margin is definitely a nice benefit as well! I'll test again a bit later.

[ignatz]

The bug was more about the pink line suddenly getting massive (see the video, was not present on master), but removing the culling margin is definitely a nice benefit as well! I'll test again a bit later.

In the video it looked like the lines were popping in/out. I didn't see the stroke width change. I (maybe falsely) assumed you had increased the border width in your testing.

[JaffaKetchup]

Yeah so I changed the width of the purple line only. I don't know why the pink line is sometimes affected and sometimes isn't. Maybe the radiusInMeters is messing something up?

[JaffaKetchup]

Unfortunately, there's still a bug with lines being culled too early. (Purple line set to strokeWidth: 100).

2024-01-22.17-47-30.mp4

[ignatz]

Thinking about it, it makes sense. One can't really treat a ~quad as a line for culling. FWIW, head should have all the same issues unless only covered up by a generous margin. I'll see if I can redo the math.

[JaffaKetchup]

On a seperate note, would it be better to make _ProjectedPoly*'s projectedPoints prop mutable? I thought we were aiming towards placing less strain on the GC?

[ignatz]

On a seperate note, would it be better to make _ProjectedPoly*'s projectedPoints prop mutable? I thought we were aiming towards placing less strain on the GC?

You mean the List? Final lists are still pretty mutable, only the reference cannot be reassigned. Constness is shallow in dart (like in many languages).

Quick update. I unfixed my "fix". The correct math is non-trivial and I definitely wouldn't want to fold this into the change. I fixed the "stroke width in meters". The early clipping of polylines should now be back to be as broken as @Head. The example app would need to pick a larger margin to prevent accidental culling.

For prosperity doing the correct math, i.e seeing if the polyline's rectangle and the axis-aligned screen rectangle (or the real non-aabb for rotated maps) is commonly addressed with what is known as "Separating Axis Theorem". For our camera.pixelBounds (which are an aabb envelope for rotated maps), this is the aabb-obb special case.

FWIW, this issue is also related to the culling TODO in hitBox detection code.

[JaffaKetchup]

You mean the List? Final lists are still pretty mutable, only the reference cannot be reassigned. Constness is shallow in dart (like in many languages).

I meant more the repetetive construction of the objects throughout the 3 stages (projection, simplification, culling). Wouldn't it be more efficient to just change the projectedPoints prop to a new list every time, rather than allocating a whole new object and cleaning up the old one?

Quick update. I unfixed my "fix". The correct math is non-trivial and I definitely wouldn't want to fold this into the change. [...] The early clipping of polylines should now be back to be as broken as @Head. The example app would need to pick a larger margin to prevent accidental culling.

For prosperity doing the correct math, i.e seeing if the polyline's rectangle and the axis-aligned screen rectangle (or the real non-aabb for rotated maps) is commonly addressed with what is known as "Separating Axis Theorem". For our camera.pixelBounds (which are an aabb envelope for rotated maps), this is the aabb-obb special case.

I have a feeling I've come across this theorem before 😂. Maybe used in FMTC somewhere, but I can't remember where. Anyway, it would be great to implement this, but I'm in agreement that another PR is the best place for this.

I fixed the "stroke width in meters".

I can't re-review right now, I'll try to do it tomorrow.

[ignatz]

You mean the List? Final lists are still pretty mutable, only the reference cannot be reassigned. Constness is shallow in dart (like in many languages).

I meant more the repetetive construction of the objects throughout the 3 stages (projection, simplification, culling). Wouldn't it be more efficient to just change the projectedPoints prop to a new list every time, rather than allocating a whole new object and cleaning up the old one?

I'm all down for reducing GC pressure, I'm just not sure I can quite put my finger on what you're having in mind.

[JaffaKetchup]

@ignatz I've made the changes in 9344c99. If you get a moment, can you let me know if what I've done makes sense and is beneficial?

[JaffaKetchup]

LGTM I think. Thanks for doing this (and sorry for getting in the way at the end again, thought I'd spotted something but I hadn't)!

[ignatz]

and sorry for getting in the way at the end again, thought I'd spotted something but I hadn't

Always appreciate your dedication. Thanks!

[JaffaKetchup]

@ignatz I think I've just found a major bug with the new simplification (for both polylines & polygons). The new values seem to be potentially dependent on DPI?

On my initial screen, the screen density is 1. On my laptop screen, it's 1.5. The performance difference is huge.

testing.mp4

[ignatz]

Interesting find. I was able to reproduce it on my laptop screen when changing the pixel scale.

I gave it my best shot: ignatz@97e6c4d . Could you try this with your setup and let me know if this does what you'd expect. When I try it, I find it a bit hard to compare since the visual appearance is so different at the respective scales. The change certainly helped in leveling out the performance differences more.