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

feat!: migrate Point<double> to Offset internally #1996

Merged
merged 54 commits into from
Dec 5, 2024

Conversation

mootw
Copy link
Contributor

@mootw mootw commented Dec 2, 2024

This will yield significant performance benefits due to a reduction in casting. This likely breaks the external API but it is worth it

Migrations:
Point<double> -> Offset or Size
Bounds<double> -> Rect
Bounds<int> -> (rename and marked internal) IntegerBounds

maybe fixes #1769 (it is not a full migration, as there are still record types (they are okay in certain conditions honestly, especially as an interface between public apis), but it does fix a TON of issues with math.Point, which is much slower than the difference between Objects and Records)

there is a modest 7% performance boost in the polygon and polyline tests in both the UI and Raster threads in my debug mode profiling. i expect other layers to benefit and more performance boost in ideal situations

@mootw mootw marked this pull request as ready for review December 3, 2024 00:41
@mootw mootw requested a review from a team December 3, 2024 03:02
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.

Some things I would be even more heavy handed/breaking on. I would be in favour of implemented deprecated methods just to ease the transition for external users, but I think we need to be more thorough.

Is there any reason for keeping Bounds using Point?

lib/src/geo/crs.dart Outdated Show resolved Hide resolved
lib/src/geo/crs.dart Outdated Show resolved Hide resolved
lib/src/gestures/map_interactive_viewer.dart Outdated Show resolved Hide resolved
lib/src/layer/polygon_layer/projected_polygon.dart Outdated Show resolved Hide resolved
lib/src/map/camera/camera.dart Outdated Show resolved Hide resolved
lib/src/map/controller/map_controller_impl.dart Outdated Show resolved Hide resolved
lib/src/misc/bounds.dart Outdated Show resolved Hide resolved
lib/src/misc/extensions.dart Outdated Show resolved Hide resolved
lib/src/misc/extensions.dart Outdated Show resolved Hide resolved
JaffaKetchup

This comment was marked as duplicate.

@JaffaKetchup JaffaKetchup changed the title Migrate to use Offset internally feat!: migrate Point<double> to Offset internally Dec 3, 2024
@mootw
Copy link
Contributor Author

mootw commented Dec 3, 2024

Some things I would be even more heavy handed/breaking on. I would be in favour of implemented deprecated methods just to ease the transition for external users, but I think we need to be more thorough.

Is there any reason for keeping Bounds using Point?

Yes, I have only migrated the Point interface methods in Bounds because the DiscreteTileRange uses Bounds<int> which would mean that i need to implement a new IntegerBounds class on top of migrating Bounds or change DiscreteTileRange to no longer require integers

@mootw mootw requested review from josxha and a team December 5, 2024 08:26
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.

Just a couple things left, mostly nits.

lib/src/geo/crs.dart Outdated Show resolved Hide resolved
lib/src/geo/crs.dart Outdated Show resolved Hide resolved
lib/src/geo/crs.dart Outdated Show resolved Hide resolved
lib/src/map/camera/camera.dart Outdated Show resolved Hide resolved
lib/src/map/camera/camera.dart Outdated Show resolved Hide resolved
mootw and others added 9 commits December 5, 2024 10:08
Co-authored-by: Luka S <github@jaffaketchup.dev>
Co-authored-by: Luka S <github@jaffaketchup.dev>
Co-authored-by: Luka S <github@jaffaketchup.dev>
@mootw mootw requested a review from JaffaKetchup December 5, 2024 16:54
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.

LGTM 🎉. Performance seems to have improved (about 0.2ms on home page example app, 0.3-0.4ms on basic polygon page, untested others), enough to actually make things feel slightly smoother (but maybe that's just a placebo). Don't notice any bugs, and code looks good.

Copy link
Contributor

@josxha josxha left a comment

Choose a reason for hiding this comment

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

I like these changes. 😄
Maybe a feature release would be good before merging breaking changes?

Edit: I wouldn't be sad if we'd not use records here, but pr is fine as is too. (:

final (minx, miny) =
        transformation.transform(b.min.dx, b.min.dy, zoomScale);
    final (maxx, maxy) =
        transformation.transform(b.max.dx, b.max.dy, zoomScale);

@mootw
Copy link
Contributor Author

mootw commented Dec 5, 2024

i like that for future changes. I want to avoid creeping scope on this PR too much for now. we could wait to release for more changes

@mootw mootw merged commit 60d9846 into fleaflet:master Dec 5, 2024
7 checks passed
@mootw mootw deleted the convert-to-offset branch December 5, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEATURE] Use classes instead of Records for efficiency
3 participants