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

feat: Maplibre #4294

Merged
merged 21 commits into from Nov 9, 2023
Merged

feat: Maplibre #4294

merged 21 commits into from Nov 9, 2023

Conversation

danieldietzler
Copy link
Member

@danieldietzler danieldietzler commented Oct 1, 2023 •
edited

Don't mind this just yet. @jrasm91 and I are testing out maplibre.

Screenshot 2023-10-18 at 22 55 28

@vercel
Copy link

vercel bot commented Oct 1, 2023 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Visit Preview Nov 8, 2023 5:00pm

@danieldietzler danieldietzler force-pushed the feat/maplibre branch 2 times, most recently from a2f4b5c to 371f22d Compare October 2, 2023 11:14
@danieldietzler danieldietzler force-pushed the feat/maplibre branch 2 times, most recently from c803eae to fc82d58 Compare October 12, 2023 17:40
@danieldietzler danieldietzler force-pushed the feat/maplibre branch 4 times, most recently from ee55e65 to 3a9ddac Compare October 16, 2023 17:39
jrasm91
jrasm91 reviewed Oct 17, 2023
View reviewed changes
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

I feel like there is a decent amount of complexity added to this by supporting a variable array of style urls, and I kind of doubt somebody is going to use more than one anyway. Thoughts on just having map.darkStyleUrl and map.lightStyleUrl. They are always there and always required. No more optional chaining, filtering, etc. I think a lot of code becomes trivial with those assumptions. I also don't think we lose hardly anything by not allowing additional styles.

server/src/domain/map/map-styles.dto.ts Outdated Show resolved Hide resolved
server/src/domain/server-info/server-info.service.spec.ts Outdated Show resolved Hide resolved
server/src/domain/system-config/system-config.service.ts Outdated Show resolved Hide resolved
@danieldietzler
Copy link
Member Author

I feel like there is a decent amount of complexity added to this by supporting a variable array of style urls, and I kind of doubt somebody is going to use more than one anyway. Thoughts on just having map.darkStyleUrl and map.lightStyleUrl. They are always there and always required. No more optional chaining, filtering, etc. I think a lot of code becomes trivial with those assumptions. I also don't think we lose hardly anything by not allowing additional styles.

I agree, however pretty much all of the logic is already done (more or less), so why throw it away? I personally don't think the complexity is too high in comparison to the functionality it brings.
Also, I can imagine people using this; there have already been requests to e.g. provide a "google-earth-like" map, so if people might want to have this and also still a "normal" map, because it loads faster or idk, they would be limited by just one style per theme

@danieldietzler danieldietzler force-pushed the feat/maplibre branch 5 times, most recently from 812d054 to 4541f03 Compare October 23, 2023 09:28
jrasm91
jrasm91 reviewed Oct 23, 2023
View reviewed changes
server/Dockerfile Outdated Show resolved Hide resolved
jrasm91
jrasm91 reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Have you mistakenly reverted the config array enhancements?

server/src/domain/system-config/system-config.core.ts Show resolved Hide resolved
@alextran1502 alextran1502 self-assigned this Nov 2, 2023
@danieldietzler
maplibre on web, custom styles from server
36c5ae1 
Actually use new vector tile server, custom style.json

support multiple style files, light/dark mode

cleanup, use new map everywhere

send file directly instead of loading first

better light/dark mode switching

remove leaflet

fix mapstyles dto, first draft of map settings

delete and add styles

fix delete default styles

fix tests

only allow one light and one dark style url

revert config core changes

fix server config store

fix tests

move axios fetches to repo

fix package-lock

fix tests
@danieldietzler
open api
061f99d
bo0tzz
bo0tzz reviewed Nov 8, 2023
View reviewed changes
server/assets/style-dark.json
@@ -1,6 +1,7 @@
{
"version": 8,
"name": "Immich Map",
"metadata": { "maputnik:renderer": "mbgljs" },
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this line in the prod style?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say no but maybe Alex ran into issues without it.

@shenlong-tanwen shenlong-tanwen force-pushed the feat/maplibre branch 4 times, most recently from 9022b6b to c73e2d6 Compare November 8, 2023 16:18
@alextran1502 alextran1502 merged commit a147dee into main Nov 9, 2023
21 checks passed
@alextran1502 alextran1502 deleted the feat/maplibre branch November 9, 2023 16:10
@waclaw66
Copy link
Contributor

waclaw66 commented Nov 9, 2023

I just updated main and it seems this causes that no map is shown. Is there any additional configuration step needed?

@jrasm91
Copy link
Contributor

jrasm91 commented Nov 9, 2023

There is a bug about it not working for non-admin users. Maybe you are running into that issue.

@waclaw66
Copy link
Contributor

waclaw66 commented Nov 9, 2023

There is a bug about it not working for non-admin users. Maybe you are running into that issue.

I checked it under my admin account.

@danieldietzler
Copy link
Member Author

@waclaw66 Is it possible that it just takes a long time to load and eventually it will show up? We're running into latency issues with the tiles server currently

@waclaw66
Copy link
Contributor

waclaw66 commented Nov 9, 2023

@danieldietzler it works now suddenly.

@danieldietzler
Copy link
Member Author

@danieldietzler it works now suddenly.

👍🏼 Yeah then that it is likely to be the same latency issue. We're working on that, sorry!

@waclaw66
Copy link
Contributor

waclaw66 commented Nov 10, 2023 •
edited

Latitude and logitude coordinates are swapped on the Open in OpenStreetMap link.

Btw. is it somehow possible to set the previous maps look? The current ones have no details and look worse. The pin is also much better then photo icon.

main 1.85.0
obrazek obrazek

@waclaw66
Copy link
Contributor

Map style settings shown as undefined and modified when settings is opened.

obrazek

@danieldietzler
Copy link
Member Author

Latitude and logitude coordinates are swapped on the Open in OpenStreetMap link.

Thanks, good catch!

Map style settings shown as undefined and modified when settings is opened.

I'm not seeing this behavior. Did you change anything?
Screenshot 2023-11-10 at 09 49 12

@waclaw66
Copy link
Contributor

waclaw66 commented Nov 10, 2023 •
edited

Didn't change anything, even when map-settings is deleted from browser settings or set by Reset button.

@danieldietzler danieldietzler mentioned this pull request Nov 11, 2023
Merged
@waclaw66 waclaw66 mentioned this pull request Nov 21, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bo0tzz bo0tzz bo0tzz left review comments

@jrasm91 jrasm91 jrasm91 approved these changes

Assignees

@alextran1502 alextran1502

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@danieldietzler @waclaw66 @jrasm91 @bo0tzz @alextran1502 @shenlong-tanwen