WIP: [indexer] Allow feature visibility override #2798

pastk · 2022-06-21T18:06:45Z

pastk commented

2022-06-21 18:06:45 +00:00

This is a WIP experimental change to allow for easy feature visibility changes without scale index or whole map regeneration.
Its meant to be generally disabled and enabled for the following use cases only:

style designers
power users whishing to use a custom map style

There are two changes:

read extra 3 scale indices - its necessary for a visibility-changed feature to be discoverable at a new lower zoom level, because its not present in the scale index at this new level
fallback to a nearest (worst) geometry if optimized geometry doesn't exist for the feature's new zoom level

This comes with some performance degradation (and, consequently, battery drain):

in my testing on a mid-range android device a slowdown was noticeable but it was comfotable to use the app still;
on a mid-range laptop a slowdown was barely noticeable;
the most of the speed impact comes from the extra scale indices read;
the impact of the geometry fallback was insignificant (but can raise if a lot more complex features will have their visibility raises compared to my test case),
this impact could be further reduced if needed by a run-time excess line points remover (we do have this code used for lines at zl [10,12] already).

So I think performance-wise its perfectly fine for a style design scenario and user testing is needed to see if power users will be happy too.

It basically replaces existing "designer tool's" functionality to regenerate a scale index (atm it does it with each feature's visibility extended by +-3 zoom levels) and additionally solves a missing geometry case (atm a full map regeneration is needed).

This is a WIP experimental change to allow for easy feature visibility changes without scale index or whole map regeneration. Its meant to be generally disabled and enabled for the following use cases only: - style designers - power users whishing to use a custom map style There are two changes: - read extra 3 scale indices - its necessary for a visibility-changed feature to be discoverable at a new lower zoom level, because its not present in the scale index at this new level - fallback to a nearest (worst) geometry if optimized geometry doesn't exist for the feature's new zoom level This comes with some performance degradation (and, consequently, battery drain): - in my testing on a mid-range android device a slowdown was noticeable but it was comfotable to use the app still; - on a mid-range laptop a slowdown was barely noticeable; - the most of the speed impact comes from the extra scale indices read; - the impact of the geometry fallback was insignificant (but can raise if a lot more complex features will have their visibility raises compared to my test case), this impact could be further reduced if needed by a run-time excess line points remover (we do have this code used for lines at zl [10,12] already). So I think performance-wise its perfectly fine for a style design scenario and user testing is needed to see if power users will be happy too. It basically replaces existing "designer tool's" functionality to regenerate a scale index (atm it does it with each feature's visibility extended by +-3 zoom levels) and additionally solves a missing geometry case (atm a full map regeneration is needed).

vng commented

2022-06-23 05:55:46 +00:00

(Migrated from github.com)

If we gonna merge it into trunk, then only with some ifdef option which is off by default.

When outdoor style will be ready, it will be merged into our styles system like others: drules_proto.bin[txt] will be the sum of all styles and its visibility is used to build scale index.

If we gonna merge it into trunk, then only with some ifdef option which is off by default. When outdoor style will be ready, it will be merged into our styles system like others: drules_proto.bin[txt] will be the sum of all styles and its visibility is used to build scale index.

pastk commented

2022-06-25 19:32:31 +00:00

Added a filter for too small areas and on/off control.

The feature is OFF by default (except for a designer tool).
It turns ON automatically on iOS and Android if a user added custom map files manually.
To turn it on in the QT app use a "?visibility-override" search command.

"?[no-]styles-override" could be additionally used on iOS/Android to use a bundled style or an added custom style, also it reloads changed custom styles files without app restart - useful for testing and style difference comparisons.
"?[no-]visibility-override" is useful for performance comparisons also.

Added a filter for too small areas and on/off control. The feature is OFF by default (except for a designer tool). It turns ON automatically on iOS and Android if a user added custom map files manually. To turn it on in the QT app use a "?visibility-override" search command. "?[no-]styles-override" could be additionally used on iOS/Android to use a bundled style or an added custom style, also it reloads changed custom styles files without app restart - useful for testing and style difference comparisons. "?[no-]visibility-override" is useful for performance comparisons also.

pastk commented

2022-06-27 14:00:52 +00:00

Updated comments/TODOs and rebased.

I think its good to go into a beta build for all users.
It will be disabled by default so no change at all for the most users except the ones who wish to test custom styles (e.g. an outdoors one).

Updated comments/TODOs and rebased. I think its good to go into a beta build for all users. It will be disabled by default so no change at all for the most users except the ones who wish to test custom styles (e.g. an outdoors one).

biodranik (Migrated from github.com) requested changes 2022-06-27 21:07:50 +00:00

biodranik (Migrated from github.com) left a comment

Комменты огонь, спасибо! Их надо в любом случае помержить.
Я против продакшн версии с этими правками. Они создают оверхед и ненужные риски, даже когда "отключены". А когда включены — тормоза, визуальные баги, быстро разряжается батарея. Позитивной обратной связи мы не получим.
Предлагаю, если есть желание, вынести все изменения в #ifdef BUILD_DESIGNER
Ещё надо встроить динамическое сканирование имеющихся стилей и их загрузку/пре-инициализацию при запуске приложения (не только из бандла ресурсов, но и из папки с данными, т.к. у нас стили будут к ним привязаны).

Хорошей альтернативой вижу наконец-то допилить динамические обновления данных, тогда можно будет спокойно подгружать альтернативные версии данных желающим потестировать.

1. Комменты огонь, спасибо! Их надо в любом случае помержить. 2. Я против продакшн версии с этими правками. Они создают оверхед и ненужные риски, даже когда "отключены". А когда включены — тормоза, визуальные баги, быстро разряжается батарея. Позитивной обратной связи мы не получим. 3. Предлагаю, если есть желание, вынести все изменения в `#ifdef BUILD_DESIGNER` 4. Ещё надо встроить динамическое сканирование имеющихся стилей и их загрузку/пре-инициализацию при запуске приложения (не только из бандла ресурсов, но и из папки с данными, т.к. у нас стили будут к ним привязаны). Хорошей альтернативой вижу наконец-то допилить динамические обновления данных, тогда можно будет спокойно подгружать альтернативные версии данных желающим потестировать.

indexer/feature_visibility.cpp

					
				@ -225,3 +225,4 @@

				  // Exclude too small area features unless it's a part of a coast or a building.

				  if (types.GetGeomType() == GeomType::Area && !types.Has(c.GetCoastType()) &&

				      !types.Has(buildingPartType) && !scales::IsGoodForLevel(level, limitRect))

biodranik (Migrated from github.com) commented

2022-06-27 21:02:10 +00:00

  // Exclude too small area features unless it's a part of a coast or a building.

```suggestion // Exclude too small area features unless it's a part of a coast or a building. ```

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

drape_frontend/rule_drawer.cpp

					
				@ -298,3 +301,4 @@

				    }

				    applyPointStyle = m_globalRect.IsPointInside(featureCenter);

				  }

vng (Migrated from github.com) commented

2022-06-28 06:02:35 +00:00

Тут все комменты в этом файле избыточны, имхо. FeatureType работает как lazy loading.
Даже если бы выше не было совсем вызовов feature::GetCenter, вызов feature::GetMinDrawableScale все равно загрузил бы геометрию и посчитал нормальный scale.

А такой стиль коммента (аля python) только усложняет чтение кода:

if (xxx)
   // comment
   yyy;

Тут все комменты в этом файле избыточны, имхо. FeatureType работает как lazy loading. Даже если бы выше не было совсем вызовов feature::GetCenter, вызов feature::GetMinDrawableScale все равно загрузил бы геометрию и посчитал нормальный scale. А такой стиль коммента (аля python) только усложняет чтение кода: ``` if (xxx) // comment yyy; ```

generator/geometry_holder.hpp

vng (Migrated from github.com) commented

2022-06-28 06:09:13 +00:00

This comment is controversial. Mask is not needed, because a feature is invisible on other scales if AddPoints was not called.

indexer/feature.cpp

vng (Migrated from github.com) commented

2022-06-28 06:11:20 +00:00

Please, put this comments above. A block without {} should have only one line expression.

// comment
if (xxx)
  yyy;

Please, put this comments above. A block without {} should have only one line expression. ``` // comment if (xxx) yyy; ```

indexer/map_style_reader.cpp

vng (Migrated from github.com) commented

2022-06-28 06:23:40 +00:00

Worth to add Platform & pl = GetPlatform(); here and below.

Worth to add ```Platform & pl = GetPlatform();``` here and below.

pastk reviewed 2022-06-28 08:34:37 +00:00

drape_frontend/rule_drawer.cpp

					
				@ -298,3 +301,4 @@

				    }

				    applyPointStyle = m_globalRect.IsPointInside(featureCenter);

				  }

pastk commented

2022-06-28 08:34:37 +00:00

Вызов feature::GetMinDrawableScale(f) загрузил бы BEST_GEOMETRY, а это ошибка.

С lazy loading всё хорошо, кроме того, что разработчику, использующему FeatureType API, неочевидно, какой вызов загружает геометрию (и главное - какую!), а какой нет. Это приводит к трудно находимым performance багам вроде #2840.

Вроде как простое открывание PP тоже приводит к загрузке BEST_GEOMETRY для выбранной фичи и её копированию, я пока не понял, зачем..

Эти комменты - попытка (да, не самая лучшая) более явно показать, где что происходит. Лучшим решением было бы, возможно, как-то хинтить в названиях методов, что вызов тянет загрузку геометрии, или убрать методы вроде GetCenter(f) и оставить только GetCenter(f, zoomLevel).

Стиль комментов поправлю.

Вызов feature::GetMinDrawableScale(f) загрузил бы BEST_GEOMETRY, а это ошибка. С lazy loading всё хорошо, кроме того, что разработчику, использующему FeatureType API, неочевидно, какой вызов загружает геометрию (и главное - какую!), а какой нет. Это приводит к трудно находимым performance багам вроде #2840. Вроде как простое открывание PP тоже приводит к загрузке BEST_GEOMETRY для выбранной фичи и её копированию, я пока не понял, зачем.. Эти комменты - попытка (да, не самая лучшая) более явно показать, где что происходит. Лучшим решением было бы, возможно, как-то хинтить в названиях методов, что вызов тянет загрузку геометрии, или убрать методы вроде GetCenter(f) и оставить только GetCenter(f, zoomLevel). Стиль комментов поправлю.

pastk reviewed 2022-06-28 08:54:08 +00:00

generator/geometry_holder.hpp

pastk commented

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

It matters for style testing/design cases only.

E.g. if line feature's visibility is changed from z15 to z14, then the lines won't appear on z14 (even after visibility/scale index regen), because there is no geometry - its kinda expected.
But actually some of the lines do appear (shorter ones)! And this is confusing.

So its not a big issue indeed but nice (and easy) to fix still.

It matters for style testing/design cases only. E.g. if line feature's visibility is changed from z15 to z14, then the lines won't appear on z14 (even after visibility/scale index regen), because there is no geometry - its kinda expected. But actually some of the lines do appear (shorter ones)! And this is confusing. So its not a big issue indeed but nice (and easy) to fix still.

pastk commented

2022-06-28 09:25:43 +00:00

2. Я против продакшн версии с этими правками. Они создают оверхед и ненужные риски, даже когда "отключены".

Could you please elaborate on overhead and risks? Maybe I miss something, but its just a few simple "if"s which disable the new code.

А когда включены — тормоза, визуальные баги, быстро разряжается батарея. Позитивной обратной связи мы не получим.

Is there any evidence for these issues?

There was one report of a slower speed on an old device, which is kind of expected. I think its fine given that this is a "hidden" feature only for people who explicitly opted in to use it.

Other users who tested the feature were quite happy and didn't complain about performance or battery life or visual bugs.

3. Предлагаю, если есть желание, вынести все изменения в `#ifdef BUILD_DESIGNER`

This way there is no possibility to test/use a changed style on a device.
Also the designer tool works on macOS only ATM.

4. Ещё надо встроить динамическое сканирование имеющихся стилей и их загрузку/пре-инициализацию при запуске приложения (не только из бандла ресурсов, но и из папки с данными, т.к. у нас стили будут к ним привязаны).

Yeap! But not in this PR of course.

Хорошей альтернативой вижу наконец-то допилить динамические обновления данных, тогда можно будет спокойно подгружать альтернативные версии данных желающим потестировать.

Looks like a lot of work to do and extra hops to jump for something that just works as easy as: put changed style files into a folder. No need to download and keep "special" map files for users, no need to generate them and keep on the servers.

> 2. Я против продакшн версии с этими правками. Они создают оверхед и ненужные риски, даже когда "отключены". Could you please elaborate on overhead and risks? Maybe I miss something, but its just a few simple "if"s which disable the new code. > А когда включены — тормоза, визуальные баги, быстро разряжается батарея. Позитивной обратной связи мы не получим. > Is there any evidence for these issues? There was one report of a slower speed on an old device, which is kind of expected. I think its fine given that this is a "hidden" feature only for people who explicitly opted in to use it. Other users who tested the feature were quite happy and didn't complain about performance or battery life or visual bugs. > 3. Предлагаю, если есть желание, вынести все изменения в `#ifdef BUILD_DESIGNER` This way there is no possibility to test/use a changed style on a device. Also the designer tool works on macOS only ATM. > 4. Ещё надо встроить динамическое сканирование имеющихся стилей и их загрузку/пре-инициализацию при запуске приложения (не только из бандла ресурсов, но и из папки с данными, т.к. у нас стили будут к ним привязаны). > Yeap! But not in this PR of course. > > Хорошей альтернативой вижу наконец-то допилить динамические обновления данных, тогда можно будет спокойно подгружать альтернативные версии данных желающим потестировать. Looks like a lot of work to do and extra hops to jump for something that just works as easy as: put changed style files into a folder. No need to download and keep "special" map files for users, no need to generate them and keep on the servers.

pastk commented

2022-06-28 17:45:45 +00:00

Added minor review fixes.

strump reviewed 2022-07-01 19:49:26 +00:00

strump left a comment

К стати, есть в проекте нагрузочные тесты, чтобы замерять скорость отрисовки, генерации карт, чтение из mwm? Чтобы можно было следить за производительностью?

indexer/map_style_reader.hpp

strump commented

2022-07-01 19:44:15 +00:00

Может назвать “SetStylesOverride”? Toggle обычно меняет флаг с true на false и наоборот.

biodranik commented

2022-07-01 21:28:48 +00:00

(Migrated from github.com)

К стати, есть в проекте нагрузочные тесты, чтобы замерять скорость отрисовки, генерации карт, чтение из mwm? Чтобы можно было следить за производительностью?

Есть benchmark_tool, он проверяет скорость декодирования геометрии на разных зумах.

> К стати, есть в проекте нагрузочные тесты, чтобы замерять скорость отрисовки, генерации карт, чтение из mwm? Чтобы можно было следить за производительностью? Есть benchmark_tool, он проверяет скорость декодирования геометрии на разных зумах.

pastk reviewed 2022-07-02 11:33:18 +00:00

generator/geometry_holder.hpp

pastk commented

2022-07-02 11:33:17 +00:00

This comment is controversial. Mask is not needed, because a feature is invisible on other scales if AddPoints was not called.

But you're right it doesn't need any change if this patch is accepted, because for style designers these mask==0 points will be used as a fallback geometry anyway.

> This comment is controversial. Mask is not needed, because a feature is invisible on other scales if AddPoints was not called. But you're right it doesn't need any change if this patch is accepted, because for style designers these mask==0 points will be used as a fallback geometry anyway.

pastk commented

2022-07-02 12:02:13 +00:00

Compacted and optimized the code a bit, added more comments and some asserts, rebased.

PTAL!

Compacted and optimized the code a bit, added more comments and some asserts, rebased. PTAL!

pastk commented

2022-07-11 09:34:20 +00:00

Rebased.

pastk commented

2022-12-15 17:20:49 +00:00

Rebased.

biodranik commented

2022-12-15 17:47:07 +00:00

(Migrated from github.com)

@pastk you're back! Welcome! 🥳 🎉 🎆

❤️ 1 🎉 1

pastk commented

2023-01-12 11:13:05 +00:00

I think its ready to merge into master long time.
Very useful for style designers and power users, no impact on other users.
@biodranik @vng

I think its ready to merge into master long time. Very useful for style designers and power users, no impact on other users. @biodranik @vng

👍 1

biodranik commented

2023-01-12 21:31:43 +00:00

(Migrated from github.com)

Здесь, очевидно, есть impact на продакшн версию для каждого пользователя, хотя реально пользоваться этой фичей будут несколько человек в мире. Можно попробовать выделить изменения в ifdef.

pastk commented

2023-01-13 14:27:12 +00:00

Здесь, очевидно, есть impact на продакшн версию для каждого пользователя, хотя реально пользоваться этой фичей будут несколько человек в мире. Можно попробовать выделить изменения в ifdef.

What is the impact exactly? I don't see how it'll affect other users. Could you elaborate please?

And making it easier even for a few style designers to contribute is important IMO.

> Здесь, очевидно, есть impact на продакшн версию для каждого пользователя, хотя реально пользоваться этой фичей будут несколько человек в мире. Можно попробовать выделить изменения в ifdef. What is the impact exactly? I don't see how it'll affect other users. Could you elaborate please? And making it easier even for a few style designers to contribute is important IMO.

biodranik commented

2023-01-13 21:22:03 +00:00

(Migrated from github.com)

What is the impact exactly? I don't see how it'll affect other users. Could you elaborate please?

Есть множество изменений не под if. Сам if тоже не комильфо для 0.0001% пользователей.

Гораздо эффективнее и проще сделать удобную версию для дизайнеров стилей на ветке, без всяких ограничений "не ломать продакшн", и собрать из неё APK для установки тем, кому это действительно нужно.

> What is the impact exactly? I don't see how it'll affect other users. Could you elaborate please? Есть множество изменений не под if. Сам if тоже не комильфо для 0.0001% пользователей. Гораздо эффективнее и проще сделать удобную версию для дизайнеров стилей на ветке, без всяких ограничений "не ломать продакшн", и собрать из неё APK для установки тем, кому это действительно нужно.

pastk commented

2023-01-15 14:17:22 +00:00

What is the impact exactly? I don't see how it'll affect other users. Could you elaborate please?

Есть множество изменений не под if. Сам if тоже не комильфо для 0.0001% пользователей.

All the code changes outside of "if"s are just refactoring, there are no behavior changes in them. All the functional changes are hidden behind "if"s.
I'm convinced still that this PR doesn't impact general users.

Гораздо эффективнее и проще сделать удобную версию для дизайнеров стилей на ветке, без всяких ограничений "не ломать продакшн", и собрать из неё APK для установки тем, кому это действительно нужно.

Unfortunately your suggestion will not work for power users and for many (most I'd say) style changes, because most of style changes go hand in hand with related changes in translations, icons, some times tests/code also. Hence to test the change in full one needs to rebuild the app anyway. And to do so with a visibility patch one needs to cherry-pick/pull it in their style change branch and then not forget to remove the patch before pushing the change...
Sorry, but all your suggestions of ifdefs, separate patch branch or special map files require additional work and attention from the style designer, while the whole point of merging the patch in is in minimizing/automating it.

Right now many style changes do not involve visibility changes because our main style is mature, so style designers don't face vis index / map regen cases too often. But with introduction of new styles there will be much more work like this, so we'll save much more time by automating.

So I ask again - let's merge it and make some users' and contributors' life easier, w/o affecting other users.

> > What is the impact exactly? I don't see how it'll affect other users. Could you elaborate please? > > Есть множество изменений не под if. Сам if тоже не комильфо для 0.0001% пользователей. > All the code changes outside of "if"s are just refactoring, there are no behavior changes in them. All the functional changes are hidden behind "if"s. I'm convinced still that this PR doesn't impact general users. > Гораздо эффективнее и проще сделать удобную версию для дизайнеров стилей на ветке, без всяких ограничений "не ломать продакшн", и собрать из неё APK для установки тем, кому это действительно нужно. Unfortunately your suggestion will not work for power users and for many (most I'd say) style changes, because most of style changes go hand in hand with related changes in translations, icons, some times tests/code also. Hence to test the change in full one needs to rebuild the app anyway. And to do so with a visibility patch one needs to cherry-pick/pull it in their style change branch and then not forget to remove the patch before pushing the change... Sorry, but all your suggestions of ifdefs, separate patch branch or special map files require additional work and attention from the style designer, while the whole point of merging the patch in is in minimizing/automating it. Right now many style changes do not involve visibility changes because our main style is mature, so style designers don't face vis index / map regen cases too often. But with introduction of new styles there will be much more work like this, so we'll save much more time by automating. So I ask again - let's merge it and make some users' and contributors' life easier, w/o affecting other users.

🚀 1

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

No reviewers

biodranik