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

Working on custom bookmark icons. #2795

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Working on custom bookmark icons. #2795

wants to merge 3 commits into from

Conversation

vng
Copy link
Member

@vng vng commented Jun 21, 2022

Accidentally closed previous one #2770.
Closes #2785

@vng vng requested a review from biodranik June 21, 2022 12:25
@vng vng force-pushed the vng-bookmarks branch 2 times, most recently from c58c9c1 to 4f42d9f Compare June 23, 2022 21:36
Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

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

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

textures->GetSymbolRegion(backgroundSymbol, backgroundRegion);
CHECK_EQUAL(region.GetTextureIndex(), backgroundRegion.GetTextureIndex(), ());
if (textures->GetSymbolRegionSafe(background, backgroundRegion))
CHECK_EQUAL(symbolRegion.GetTextureIndex(), backgroundRegion.GetTextureIndex(), ());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

dp::TextureManager::SymbolRegion region;
textures->GetSymbolRegion(symbolName, region);
symbolSize = region.GetPixelSize();
textures->GetSymbolRegion(symbolName, symbolRegion);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@vng vng Jun 28, 2022

Choose a reason for hiding this comment

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

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

}
catch (RootException & e)
{
return false;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


int bpp;
m_data = stbi_load_from_memory(buffer.data(), static_cast<int>(buffer.size()), &m_width, &m_height, &bpp, 0);
if (m_data && bpp == 4) // only this fits TextureFormat::RGBA8
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -120,10 +122,11 @@ std::string GetSymbolNameForZoomLevel(ref_ptr<UserPointMark::SymbolNameZoomInfo>
if (!symbolNames)
return {};

for (auto itName = symbolNames->crbegin(); itName != symbolNames->crend(); ++itName)
auto const & v = symbolNames->m_zoomInfo;
for (auto i = v.crbegin(); i != v.crend(); ++i)
Copy link
Member

Choose a reason for hiding this comment

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

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

fileSavePath = GenerateValidAndUniqueFilePathForKML(file);
ZipFileReader::UnzipFile(filePath, file, fileSavePath);
}
else if (ext == ".png")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -296,7 +290,9 @@ std::unique_ptr<kml::FileData> LoadKmlFile(std::string const & file, KmlFileType

std::string GetKMLPath(std::string const & filePath)
Copy link
Member

Choose a reason for hiding this comment

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

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

if (!bmData.m_iconPath.empty())
{
// Check that user icon file is actually present.
if (!GetPlatform().IsFileExistsByFullPath(base::JoinPath(GetBookmarksDirectory(), bmData.m_iconPath)))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

{
TEST(base::DeleteFileX(filePath), ());
// Cleanup 'files' folder from "test.kmz".
TEST(Platform::RmDirRecursively(pngDir), ());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -1338,7 +1337,7 @@ Java_com_mapswithme_maps_Framework_nativeSetMapStyle(JNIEnv * env, jclass, jint
}

JNIEXPORT jint JNICALL
Java_com_mapswithme_maps_Framework_nativeGetMapStyle(JNIEnv * env, jclass, jint mapStyle)
Java_com_mapswithme_maps_Framework_nativeGetMapStyle(JNIEnv * env, jclass)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

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

@vng
Copy link
Member Author

vng commented Aug 28, 2022

Давай: #3282

@biodranik
Copy link
Member

Rebase?

@biodranik biodranik added Icons Map and app icons Bookmarks and Tracks Bookmarks, imported tracks, and KML, KMZ, KMB, GPX, GPZ import or export labels Dec 5, 2022
@biodranik biodranik mentioned this pull request Mar 28, 2023
@biodranik
Copy link
Member

To better support different formats, a newer version of stb_image.h should be used: https://github.com/nothings/stb/blob/master/stb_image.h

Signed-off-by: Viktor Govako <viktor.govako@gmail.com>
Signed-off-by: Viktor Govako <viktor.govako@gmail.com>
Signed-off-by: Viktor Govako <viktor.govako@gmail.com>
@Jean-BaptisteC
Copy link
Member

Export is broken on Android ?
I didn't see file when I export kml to app mail or messaging app

@vng vng marked this pull request as draft October 10, 2023 17:37
@vng
Copy link
Member Author

vng commented Oct 10, 2023

I just did a rebase on the latest master. This is a draft :)

@Jean-BaptisteC
Copy link
Member

Ok, that's weird when I export bookmarks, I have a Memory Warning

MwmActivity app.organicmaps.debug D  MwmActivity.java:2217 onTrimMemory(): trim memory, level = 20
OMcore app.organicmaps.debug I  map/framework.cpp:1158 MemoryWarning(): MemoryWarning

@biodranik
Copy link
Member

Important requirements for this PR:

  1. KMZ Import with custom png or jpg icons should work. SVG support would be awesome next step.
  2. Imported images should be shared (available for custom bookmark icons in other lists)
  3. Importing some images should not delete/rewrite/replace existing images (by default, most KMZ files have icon.png, or image.png)
  4. The texture size should not be hard-coded but increased dynamically when necessary. Users will import A LOT of bookmarks, it should not crash or break OM.
  5. KMZ export should work as expected, with custom icons included, to be easily edited in Google Earth and other existing editors (umap?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bookmarks and Tracks Bookmarks, imported tracks, and KML, KMZ, KMB, GPX, GPZ import or export Icons Map and app icons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[kml, kmz, gpz] Custom symbols support for imported kmz places.
3 participants