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.

Sign up for GitHub

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

Jump to bottom

perf: avoid caching a single multiplication #1743

Merged
merged 4 commits into from
Dec 2, 2023
Merged

perf: avoid caching a single multiplication #1743

merged 4 commits into from
Dec 2, 2023

Conversation

mootw
Copy link
Collaborator

@mootw mootw commented Dec 2, 2023

avoids caching a single multiplication operation. converts all toRadians and other helpers to use vector_math constant. (part of reducing dependency on latlong2).

TesteurManiak
TesteurManiak approved these changes Dec 2, 2023
View reviewed changes
Copy link
Collaborator

@TesteurManiak TesteurManiak left a comment

Choose a reason for hiding this comment

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

LGTM!

JaffaKetchup
JaffaKetchup requested changes Dec 2, 2023
View reviewed changes
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 files import '_64', some don't? Is that intentional?

@josxha josxha mentioned this pull request Dec 2, 2023
Merged
3 tasks
@josxha
Copy link
Contributor

josxha commented Dec 2, 2023 •
edited
Loading

As far as I can see we had vector_math as a dependency but it was not used anywhere?
This PR introduces the use of it for the degrees2Radians constant. Are there additional use cases in the future? If not I'd prefer to remove the dependency entirely and add the const to flutter_map directly.

Long terms I think it should be used from a future latlong package but not the current implementation of latlong2.

@JaffaKetchup
Copy link
Member

I would agree with @josxha here - adding a dependency just for this seems a little bit too much.

@josxha josxha added this to the v6.1 milestone Dec 2, 2023
@mootw
Copy link
Collaborator Author

mootw commented Dec 2, 2023

vector_math is a core flutter dependency, and we already use it in camera.dart for the matrix transform at line 301, we just don't explicitly depend on it. Matrix4 is exposed by package:flutter/material.dart

JaffaKetchup
JaffaKetchup approved these changes Dec 2, 2023
View reviewed changes
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! I suppose that we indirectly depend on it anyway, so no harm in depending on it directly as well.

Would be great if you could resolve the conflicts as well :D.

@JaffaKetchup JaffaKetchup changed the title avoid caching a single multiplication perf: avoid caching a single multiplication Dec 2, 2023
josxha
josxha approved these changes Dec 2, 2023
View reviewed changes
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.

thanks for the explanation. pr lgtm

@JaffaKetchup JaffaKetchup merged commit df40d8c into fleaflet:master Dec 2, 2023
6 checks passed
@mootw
Copy link
Collaborator Author

mootw commented Dec 2, 2023

np! also, we could consider using the aabb2 and other methods from vector_math in the future for things like bounding boxes and transforms!

@JaffaKetchup
Copy link
Member

(@josxha feel free to cherry pick)

@mootw mootw deleted the feat/remove_caching_and_rad_function branch December 2, 2023 16:51
@JaffaKetchup
Copy link
Member

np! also, we could consider using the aabb2 and other methods from vector_math in the future for things like bounding boxes and transforms!

Yep, that would make sense.

josxha pushed a commit that referenced this pull request Dec 2, 2023
@mootw @josxha
perf: avoid caching a single multiplication (#1743)
8200780
josxha added a commit that referenced this pull request Dec 6, 2023
@josxha
Squashed commit of the following:
c4df50e 
commit ec81782
Author: Luka S <github@jaffaketchup.dev>
Date:   Sat Dec 2 20:48:08 2023 +0000

    chore: v6.1.0 release preparation (#1749)

commit df40d8c
Author: mootw <spencer@xqkz.net>
Date:   Sat Dec 2 10:49:29 2023 -0600

    perf: avoid caching a single multiplication (#1743)

commit ad8318b
Author: Joscha <34318751+josxha@users.noreply.github.com>
Date:   Sat Dec 2 16:39:21 2023 +0100

    refactor: example app plugins (#1744)

commit c0829b4
Author: Joscha <34318751+josxha@users.noreply.github.com>
Date:   Sat Dec 2 16:31:38 2023 +0100

    fix: `MapPosition.hashCode` value distribution (#1747)

commit 51d3eda
Author: Luka S <github@jaffaketchup.dev>
Date:   Sat Dec 2 11:34:13 2023 +0000

    revert: #1731 (#1745)

    Revert #1731
@josxha josxha mentioned this pull request Dec 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TesteurManiak TesteurManiak TesteurManiak approved these changes

@josxha josxha josxha approved these changes

@JaffaKetchup JaffaKetchup JaffaKetchup approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Development
Archived in project
Milestone
v6.1
Development

Successfully merging this pull request may close these issues.
4 participants
@mootw @josxha @JaffaKetchup @TesteurManiak