-
-
Notifications
You must be signed in to change notification settings - Fork 860
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
perf!: remove unnecessary rounding and surrounding features, and produce less garbage #1714
Conversation
(2) and (3) break public APIs 😂. I won't give my full thoughts or review now, as I'm on a phone with 8% battery, but I'll look at it this evening. However... |
I'd be very wary about not rounding pixelOrigin without a good reason. Leaflet.js has done this forever iirc, and flutter_map has done this pretty much forever, and floats often lead to odd bugs (not specifically meaning flutter_map here). Here is one example in leaflet for the reasoning https://stackoverflow.com/questions/60412852/is-there-any-reason-why-leaflet-rounds-pixel-coordinates (I'm not sure if flutter->web would have this issue or not). I also suspect odd errors from things like this when I see narrow white lines between tiles (I note google maps suffers from this now and it bugs me :) ) There are some potential advantages I think like that issue says about animations potentially. Just be careful with it, without a lot of testing and thought as to how others use calculations based on the concept that its currently rounded. |
@ibrierley testing sounds good. FWIW, it's a bit more tricky especially when it comes to what FlutterMap has done forever. I'm removing the rounding from I certainly agree, let's all be diligent. |
@ignatz is this ready for review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although this is breaking, it's very minor (num
to double
for only a small number of users). Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think everything is OK in terms of rounding changes @ibrierley.
I think I'm OK with the breaking changes here as well. LGTM, thanks!
Sorry, this combines a bunch of things that could be pulled apart. This is the result of me going on a spree trying to speed up Polygons, which are still the slowest experience in my App.
In a nutshell:
scaleBy
is not a vector scaling operation. I don't know what it is or what it would ever be used for. So I removed it as well.This is were @JaffaKetchup is hopefully going to say: "(2) and (3) break public APIs" :)
I'd argue that (2) is a bug that needs fixing. Whereas (3) is negotiable. Note that I left the
Point<double>.subtract
method, which I presume is the only one ever used to deal with (2). I definitely don't think we're setting a good example by helping dig a hole into type-safety :).