organicmaps/organicmaps-tmp

Archived

WIP: Working on custom bookmark icons. #2795

Draft

vng wants to merge 3 commits from vng-bookmarks into master

vng commented

2022-06-21 12:25:07 +00:00

(Migrated from github.com)

Accidentally closed previous one organicmaps/organicmaps#2770.
Closes organicmaps/organicmaps#2785

Accidentally closed previous one https://git.omaps.dev/organicmaps/organicmaps/pulls/2770. Closes https://git.omaps.dev/organicmaps/organicmaps/issues/2785

biodranik (Migrated from github.com) reviewed 2022-06-27 23:05:03 +00:00

biodranik (Migrated from github.com) left a comment

Тут много рефакторингов и улучшений, которые надо вынести и помержить отдельно, не затягивая, например jni.

Остальное обсудим, будем делать итерационно, я бы начал с нормальных стилей.

Тут много рефакторингов и улучшений, которые надо вынести и помержить отдельно, не затягивая, например jni. Остальное обсудим, будем делать итерационно, я бы начал с нормальных стилей.

android/jni/com/mapswithme/maps/Framework.cpp

biodranik (Migrated from github.com) commented

2022-06-27 23:03:44 +00:00

env тоже удалить тогда

env тоже удалить тогда

drape/symbols_texture.cpp

biodranik (Migrated from github.com) commented

2022-06-25 05:40:03 +00:00

Штатно? Лог не нужен?

Штатно? Лог не нужен?

drape/symbols_texture.cpp

biodranik (Migrated from github.com) commented

2022-06-25 05:41:15 +00:00

Т.е. мы не все картинки сможем поддерживать?

Т.е. мы не все картинки сможем поддерживать?

drape_frontend/user_mark_shapes.cpp

					
				@ -118,10 +120,11 @@ std::string GetSymbolNameForZoomLevel(ref_ptr<UserPointMark::SymbolNameZoomInfo>

				  if (!symbolNames)

				    return {};

biodranik (Migrated from github.com) commented

2022-06-25 05:43:12 +00:00

Лучше it для итераторов, а i для индексов, тогда проще код читать.

Лучше it для итераторов, а i для индексов, тогда проще код читать.

drape_frontend/user_mark_shapes.cpp

biodranik (Migrated from github.com) commented

2022-06-23 22:25:10 +00:00

Зачем нужна небезопасная версия без проверки на возвращаемое значение? Кстати, может std::optional будет лучше?

Зачем нужна небезопасная версия без проверки на возвращаемое значение? Кстати, может std::optional будет лучше?

drape_frontend/user_mark_shapes.cpp

					
				@ -397,24 +400,34 @@ void CacheUserMarks(ref_ptr<dp::GraphicsContext> context, TileKey const & tileKe

				    m2::PointD const tileCenter = tileKey.GetGlobalRect().Center();

biodranik (Migrated from github.com) commented

2022-06-25 05:43:41 +00:00

to the default

to the default

drape_frontend/user_mark_shapes.cpp

					
				@ -406,0 +414,4 @@

				          LOG(LWARNING, ("Can't load symbol:", symbolName));

				          // Fallback to the default bookmark's icon if user icon's file was not loaded.

				          symbolName = "bookmark-default-m";

biodranik (Migrated from github.com) commented

2022-06-23 22:24:16 +00:00

Это точно нужно в релизе?

Это точно нужно в релизе?

drape_frontend/user_mark_shapes.cpp

biodranik (Migrated from github.com) commented

2022-06-25 05:44:22 +00:00

Нам понадобятся подробные логи, чтобы понять и исправить основные причины ошибок с разными юзерскими метками.

Нам понадобятся подробные логи, чтобы понять и исправить основные причины ошибок с разными юзерскими метками.

biodranik (Migrated from github.com) commented

2022-06-27 22:34:36 +00:00

При импорте часто случается дефолтный цвет, зачем его на none менять для обычных меток без иконок?

При импорте часто случается дефолтный цвет, зачем его на none менять для обычных меток без иконок?

					
				@ -187,3 +184,4 @@

				         << indent << kIndent8 << "<href>" << style << "</href>\n"

				         << indent << kIndent6 << "</Icon>\n"

				         << indent << kIndent4 << "</IconStyle>\n"

				         << indent << kIndent2 << "</Style>\n";

biodranik (Migrated from github.com) commented

2022-06-27 22:35:14 +00:00

Зачем ломать? Там же наши иконки зашиты, чтобы в Google Earth показывались.

Зачем ломать? Там же наши иконки зашиты, чтобы в Google Earth показывались.

biodranik (Migrated from github.com) commented

2022-06-27 22:37:55 +00:00

А многие остальные будут указывать урлы к своим иконкам.

А многие остальные будут указывать урлы к своим иконкам.

biodranik (Migrated from github.com) commented

2022-06-27 22:38:24 +00:00

?

?

biodranik (Migrated from github.com) commented

2022-06-27 22:40:16 +00:00

Сейчас стили шарятся между всеми сетами? Если нет, то надо будет шарить, чтобы оверхеда не было.

Сейчас стили шарятся между всеми сетами? Если нет, то надо будет шарить, чтобы оверхеда не было.

					
				@ -972,20 +981,22 @@ void KmlParser::Pop(std::string_view tag)

				           (tag == "mwm:additionalLineStyle" && m_tags.size() > 3 && GetTagFromEnd(3) == kPlacemark))

biodranik (Migrated from github.com) commented

2022-06-27 22:41:07 +00:00

А дефолтные треки не сломаются?

А дефолтные треки не сломаются?

biodranik (Migrated from github.com) commented

2022-06-27 22:44:53 +00:00

По коду выглядит плохой идеей смешивать цвета с иконками. Лучше сразу делать правильно, чтобы поддерживать разные стили из KML и GPX, но на первом шаге закодированные цвета/заливки и внешние картинки.

По коду выглядит плохой идеей смешивать цвета с иконками. Лучше сразу делать правильно, чтобы поддерживать разные стили из [KML](https://developers.google.com/kml/documentation/kmlreference#style) и [GPX](https://www.topografix.com/GPX/gpx_style/0/2/), но на первом шаге закодированные цвета/заливки и внешние картинки.

					
				@ -1169,0 +1178,4 @@

				          TrackLayer layer;

				          layer.m_lineWidth = style->width;

				          layer.m_color.m_predefinedColor = PredefinedColor::None;

biodranik (Migrated from github.com) commented

2022-06-27 22:46:28 +00:00

<Style> is correct official format

`<Style>` is [correct official format](https://developers.google.com/kml/documentation/kmlreference#style)

biodranik (Migrated from github.com) commented

2022-06-27 22:48:52 +00:00

<styleUrl> внутри <Placemark> тоже официальный формат.

`<styleUrl>` внутри `<Placemark>` тоже [официальный формат](https://developers.google.com/kml/documentation/kmlreference#placemark).

biodranik (Migrated from github.com) commented

2022-06-27 22:49:46 +00:00

А вот Style внутри Placemark уже нарушение, как пишут в комментах. Но видимо придётся поддерживать, кто ж это так накосячил...

А вот Style внутри Placemark уже нарушение, как пишут в комментах. Но видимо придётся поддерживать, кто ж это так накосячил...

biodranik (Migrated from github.com) commented

2022-06-27 22:50:17 +00:00

Да вообще жесть какая-то.

Да вообще жесть какая-то.

biodranik (Migrated from github.com) commented

2022-06-27 22:50:43 +00:00

А что в else?

А что в else?

					
				@ -118,0 +120,4 @@

				    double width = kDefaultWidth;

				    std::string iconPath;

				    void Invalidate()

biodranik (Migrated from github.com) commented

2022-06-27 22:52:39 +00:00

Разве FFFFFFFF не белый цвет?

Разве FFFFFFFF не белый цвет?

biodranik (Migrated from github.com) commented

2022-06-27 22:53:26 +00:00

Вообще, наш invalid color это костыль, цвета или иконки по идее всегда должны быть в прилетающих к нам в kml данных.

Вообще, наш invalid color это костыль, цвета или иконки по идее всегда должны быть в прилетающих к нам в kml данных.

biodranik (Migrated from github.com) commented

2022-06-27 22:54:04 +00:00

А чего путь туда же не всунуть?

А чего путь туда же не всунуть?

map/bookmark_helpers.cpp

biodranik (Migrated from github.com) commented

2022-06-27 22:59:32 +00:00

Это геттер, который много всего внутри делает, надо его отрефакторить.

Это геттер, который много всего внутри делает, надо его отрефакторить.

map/bookmark_helpers.cpp

biodranik (Migrated from github.com) commented

2022-06-27 22:57:34 +00:00

В корне будет плохо держать картинки, при их большом количестве сильно замедлится загрузка меток.

В корне будет плохо держать картинки, при их большом количестве сильно замедлится загрузка меток.

map/bookmark_manager.cpp

biodranik (Migrated from github.com) commented

2022-06-27 23:00:42 +00:00

А зачем именно в CreateBookmark это проверять?

А зачем именно в CreateBookmark это проверять?

map/map_tests/kmz_unarchive_test.cpp

					
				@ -3,39 +3,61 @@

				#include "map/bookmark_helpers.hpp"

biodranik (Migrated from github.com) commented

2022-06-27 23:01:40 +00:00

Эти тесты поудаляют другие десктопные метки для работы/других кейзов?

Эти тесты поудаляют другие десктопные метки для работы/других кейзов?

vng (Migrated from github.com) reviewed 2022-06-28 06:43:08 +00:00

drape_frontend/user_mark_shapes.cpp

					
				@ -406,0 +414,4 @@

				          LOG(LWARNING, ("Can't load symbol:", symbolName));

				          // Fallback to the default bookmark's icon if user icon's file was not loaded.

				          symbolName = "bookmark-default-m";

vng (Migrated from github.com) commented

2022-06-28 06:43:08 +00:00

У нас в целом стратегия предпочтения CHECK там где не критично по производительности.
В drape это имеет смысл, потому что много зависит от зоопарка девайсов.

У нас в целом стратегия предпочтения CHECK там где не критично по производительности. В drape это имеет смысл, потому что много зависит от зоопарка девайсов.

vng (Migrated from github.com) reviewed 2022-06-28 06:49:00 +00:00

drape_frontend/user_mark_shapes.cpp

vng (Migrated from github.com) commented

2022-06-28 06:49:00 +00:00

Она все также проверяет, просто дает дополнительный лог.
Я не сторонник огульного переписывания на optional, тем более что это ничего не дает (ок синтаксис красивее), а иногда еще и ухудшит :)

- Она все также проверяет, просто дает дополнительный лог. - Я не сторонник огульного переписывания на optional, тем более что это ничего не дает (ок синтаксис красивее), а иногда еще и ухудшит :)

vng (Migrated from github.com) reviewed 2022-06-28 06:51:08 +00:00

drape/symbols_texture.cpp

vng (Migrated from github.com) commented

2022-06-28 06:51:08 +00:00

Не нужен, входная строка может быть не обязательно файл.

Не нужен, входная строка может быть не обязательно файл.

vng (Migrated from github.com) reviewed 2022-06-28 06:51:51 +00:00

drape/symbols_texture.cpp

vng (Migrated from github.com) commented

2022-06-28 06:51:51 +00:00

Если только делать какую-то "конверсию".

Если только делать какую-то "конверсию".

vng (Migrated from github.com) reviewed 2022-06-28 06:57:20 +00:00

vng (Migrated from github.com) commented

2022-06-28 06:57:19 +00:00

Патч цвета есть после импорта, который проверяет более сложное условие с учетом кастомной иконки. Здесь важно чтобы функция вернула как есть - цвет не найден.

Патч цвета есть после импорта, который проверяет более сложное условие с учетом кастомной иконки. Здесь важно чтобы функция вернула как есть - цвет не найден.

vng (Migrated from github.com) reviewed 2022-06-28 06:59:29 +00:00

					
				@ -187,3 +184,4 @@

				         << indent << kIndent8 << "<href>" << style << "</href>\n"

				         << indent << kIndent6 << "</Icon>\n"

				         << indent << kIndent4 << "</IconStyle>\n"

				         << indent << kIndent2 << "</Style>\n";

vng (Migrated from github.com) commented

2022-06-28 06:59:29 +00:00

Ничего тут не ломается. Для цветовых иконок все останется как было, но у нас уже есть кастомные иконки, которые не https://omaps..., а images/symbol.png

Ничего тут не ломается. Для цветовых иконок все останется как было, но у нас уже есть кастомные иконки, которые не https://omaps..., а images/symbol.png

vng (Migrated from github.com) reviewed 2022-06-28 07:00:47 +00:00

vng (Migrated from github.com) commented

2022-06-28 07:00:47 +00:00

Это коммент не про то или я не понял твой коммент.

Это коммент не про то или я не понял твой коммент.

vng (Migrated from github.com) reviewed 2022-06-28 07:01:36 +00:00

vng (Migrated from github.com) commented

2022-06-28 07:01:36 +00:00

ненужная лишняя строка

ненужная лишняя строка

vng (Migrated from github.com) reviewed 2022-06-28 07:03:29 +00:00

vng (Migrated from github.com) commented

2022-06-28 07:03:29 +00:00

Как ты представляешь шаринг стилей между всеми сетами? У нас в загруженном сете нет понятия стиля.
Стиль есть только когда сет пишется в файл, каждый файл независим.

Как ты представляешь шаринг стилей между всеми сетами? У нас в загруженном сете нет понятия стиля. Стиль есть только когда сет пишется в файл, каждый файл независим.

vng (Migrated from github.com) reviewed 2022-06-28 07:04:53 +00:00

					
				@ -972,20 +981,22 @@ void KmlParser::Pop(std::string_view tag)

				           (tag == "mwm:additionalLineStyle" && m_tags.size() > 3 && GetTagFromEnd(3) == kPlacemark))

vng (Migrated from github.com) commented

2022-06-28 07:04:53 +00:00

Тут функция стала полностью другая по назначению.

Тут функция стала полностью другая по назначению.

vng (Migrated from github.com) reviewed 2022-06-28 07:07:29 +00:00

vng (Migrated from github.com) commented

2022-06-28 07:07:29 +00:00

"неправильно-правильно" тут не корректный комментарий. У нас одна структура BookmarksData для любой entity скажем так, где-то присваивается цвет, где-то ссылка на иконку.

"неправильно-правильно" тут не корректный комментарий. У нас одна структура BookmarksData для любой entity скажем так, где-то присваивается цвет, где-то ссылка на иконку.

vng (Migrated from github.com) reviewed 2022-06-28 07:08:55 +00:00

					
				@ -1169,0 +1178,4 @@

				          TrackLayer layer;

				          layer.m_lineWidth = style->width;

				          layer.m_color.m_predefinedColor = PredefinedColor::None;

vng (Migrated from github.com) commented

2022-06-28 07:08:55 +00:00

Комментарий об этом говорил и говорит.

Комментарий об этом говорил и говорит.

vng (Migrated from github.com) reviewed 2022-06-28 07:10:38 +00:00

					
				@ -118,0 +120,4 @@

				    double width = kDefaultWidth;

				    std::string iconPath;

				    void Invalidate()

vng (Migrated from github.com) commented

2022-06-28 07:10:38 +00:00

Белый прозрачный - по сути нет цвета.
Не костыль, это значит цвета нет и он не проинициализирован.

Белый прозрачный - по сути нет цвета. Не костыль, это значит цвета нет и он не проинициализирован.

vng (Migrated from github.com) reviewed 2022-06-28 07:11:01 +00:00

vng (Migrated from github.com) commented

2022-06-28 07:11:01 +00:00

ничего

ничего

vng (Migrated from github.com) reviewed 2022-06-28 07:13:14 +00:00

map/bookmark_helpers.cpp

vng (Migrated from github.com) commented

2022-06-28 07:13:14 +00:00

Они будут лежать там где указано в kml, обычно это images, files, ...
Сильно не замедлится.

- Они будут лежать там где указано в kml, обычно это images, files, ... - Сильно не замедлится.

vng (Migrated from github.com) reviewed 2022-06-28 07:15:14 +00:00

vng (Migrated from github.com) commented

2022-06-28 07:15:14 +00:00

BookmarkIcon - predefined enum на наши внутренние иконки. Тут можно было бы переименовать BookmarkIcon, потом добавить уже объединяющую структуру, но не вижу профита.

BookmarkIcon - predefined enum на наши внутренние иконки. Тут можно было бы переименовать BookmarkIcon, потом добавить уже объединяющую структуру, но не вижу профита.

vng (Migrated from github.com) reviewed 2022-06-28 07:18:10 +00:00

map/bookmark_manager.cpp

vng (Migrated from github.com) commented

2022-06-28 07:18:10 +00:00

Сейчас это самое логично место. Возможно будет по другому, когда кто-то перепишет все :)

Сейчас это самое логично место. Возможно будет по другому, когда кто-то перепишет все :)

vng (Migrated from github.com) reviewed 2022-06-28 07:21:02 +00:00

map/map_tests/kmz_unarchive_test.cpp

					
				@ -3,39 +3,61 @@

				#include "map/bookmark_helpers.hpp"

vng (Migrated from github.com) commented

2022-06-28 07:21:02 +00:00

Да, тут стоит пересмотреть логику.

Да, тут стоит пересмотреть логику.

biodranik (Migrated from github.com) reviewed 2022-08-27 20:40:36 +00:00

biodranik (Migrated from github.com) left a comment

Давай вмержим 1-4, 7, 9 коммиты?

Давай вмержим 1-4, 7, 9 коммиты?

vng commented

2022-08-28 06:07:53 +00:00

(Migrated from github.com)

Давай: organicmaps/organicmaps#3282

Давай: https://git.omaps.dev/organicmaps/organicmaps/pulls/3282

biodranik commented

2022-08-28 20:51:05 +00:00

(Migrated from github.com)

Rebase?

Rebase?

This repo is archived. You cannot comment on pull requests.

No reviewers

No labels

Android Automotive (AAOS)

Android Automotive (AAOS)

Battery and Performance

Battery and Performance

Bookmarks and Tracks

Bookmarks and Tracks

External Map Datasets

External Map Datasets

Frequently User Reported

Frequently User Reported

Good first issue

Good first issue

Linux packaging

Linux packaging

NLnet 2024-06-281

NLnet 2024-06-281

No Feature Parity

No Feature Parity

Public Transport

Public Transport

Track Recording

Track Recording

Walk Navigation

Walk Navigation

No milestone

No project

No assignees

1 participant

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#2795

No description provided.