perf!: cache projection of polygon points & CRS improvements

[ignatz]

RFC based on #1793

[JosefWN]

Cool! Will try out the PR with my CRS for a better feel soon, just adding some initial thoughts.

[JaffaKetchup]

Wow, some great changes, thanks :) Few minor changes, and I'll make a couple basic formatting changes.
After the research into Records which revealed they were innefficient, should we be using them? I believe you were there or acknowledged it, but I'll try to dig it out if not.

[JaffaKetchup]

This PR appears to be split into two parts, CRS improvements and Polygon improvements. Should they be seperated into seperate PRs? (no worries if that's too time consuming.)

[ignatz]

After the research into Records which revealed they were innefficient, should we be using them? I believe you were there or acknowledged it, but I'll try to dig it out if not.

Yeah, pretty much. I was torn. I started with a Record (especially since some of the CRS methods already use records) but at the end:

• they have extra memory overhead (that was the interesting find)
• they are not as convenient in terms of extensible API (even with extension methods).
• and records are immutable and mutability is a fence we might wanna jump some day.

Though the last points is something we should certainly discuss more in terms of safety if we decide to make PointDouble public. At the moment I'm only really using it internally, i.e. we can change it w/o a major version change.

This PR appears to be split into two parts, CRS improvements and Polygon improvements. Should they be seperated into seperate PRs? (no worries if that's too time consuming.)

Once could, though I'd argue that CRS changes are very minor (it's mostly just adding (un)transform). If we'd split them we'd add new APIs w/o a user.

[ignatz]

Thanks for the reformatting. Quick remark, if you guys feel strongly about style i'd suggest installing or writing linter rules for it. Overall that will reduce churn, enforce the preferences more consistently across the code-base, and tell authors what you're looking for.

[JaffaKetchup]

Quick remark, if you guys feel strongly about style i'd suggest installing or writing linter rules for it.

Yeah we've been slowly adding them (although not for a while). One I feel strongly about is trailing commas, but I know that's debated :D

[JaffaKetchup]

Ok, so I think this just needs the conversation with @JosefWN resolved, and the same changes as applied to the polygons applied to the polylines as well (as I believe some of the changes have affected polyline simplification, but the other changes have not been made).

[ignatz]

Ok, so I think this just needs the conversation with @JosefWN resolved,

Feel free to weigh in, always welcome a tie breaker. As far as I can tell, there are two conversations interleaved:

• As we're making _CrsWithStaticTransformation package visible, should we give it a better name.
• Should we use math.Point instead of PointDouble? I'll ignore the conversation around whether we should use PointDouble in the public APIs because that's a much bigger and breaking change.

Personally, I'm happy to do any rename, however feel strongly about not using math.Point especially in internal APIs since it goes against the performance optimizations we're trying to do here 🤷

as I believe some of the changes have affected polyline simplification, but the other changes have not been made

This change should not affect polyline. We should be able to make these changes independently.

[JaffaKetchup]

As we're making _CrsWithStaticTransformation package visible, should we give it a better name.

I'll opt out of discussing this one, I'm not even remotely qualified :D

Should we use math.Point instead of PointDouble? I'll ignore the conversation around whether we should use PointDouble in the public APIs because that's a much bigger and breaking change.

I think you're right, given that the entire point of these changes is performance, we might as well squeeze as much out of this as possible. It's not ideal, and could be confusing, but it's documented well enough.

This change should not affect polyline. We should be able to make these changes independently.

I'm not sure whether it affects polylines directly - the simplify methods have been changed to accept a pixel tolerance. In any case, the same changes need to be made to the polylines for consistency to reduce confusion between the two.

[ignatz]

I'm not sure whether it affects polylines directly - the simplify methods have been changed to accept a pixel tolerance. In any case, the same changes need to be made to the polylines for consistency to reduce confusion between the two.

Nothing should have changed for polylines. Simplify is unit agnostic. At the moment:

• Polygons feed projected points and and untransformed tolerance
• polygons feed latlng points and latlng tolerance.

I was hoping to do polylines in a follow up. WDYT?

[JaffaKetchup]

Ah OK, my misunderstanding, in that case I'm happy for it to be delayed out of this PR, but it does need to be done before the v7 release.

[JosefWN]

As we're making _CrsWithStaticTransformation package visible, should we give it a better name.

Perhaps at least a doc comment stating the intended purpose of the class?

But I will leave this PR in the hands of the maintainers, there isn't that much more I can add.

[JaffaKetchup]

Perhaps at least a doc comment stating the intended purpose of the class?

That does seem reasonable.

[ignatz]

Perhaps at least a doc comment stating the intended purpose of the class?

Added a docstring, annotated it explicitly internal, and am not exporting it. Hope that addresses any concerns.

[JaffaKetchup]

I think LGTM, thanks!

[josxha]

Tested the pull request on windows and found no bugs.

In my opinion all functions in simplify.dart should have at least a doc string for better documentation.
e170a1a and 9e07d1f removed two doc strings without replacement.

Other than that the pr looks good to me.

[JaffaKetchup]

I committed e170a1a and 9e07d1f. I removed the doc strings because they are internal functions, and have relatively self documenting names and bodies:

• // simplification using Ramer-Douglas-Peucker algorithm on simplifyDouglasPeucker, and this is explained in online documentation
• /// high quality simplification uses the Ramer-Douglas-Peucker algorithm otherwise it just merges close points on simplify when the method body is short and makes this specific comparison: highQuality ? simplifyDouglasPeucker(...) : simplifyRadialDist(...)

I'm happy to add them back in another PR @josxha, when I/we more strongly revise the documentation.

[josxha]

If you want to documentation in a follow up pull request it's fine for me - althrough it shouldn't get delayed to much or someone unlucky in the future needs to dig through these complex undocumented functions.