WIP: Fetch sidewalk name from nearest road #8975

RedAuburn · 2024-08-15T19:25:24Z

RedAuburn commented

2024-08-15 19:25:24 +00:00

Don't store sidewalk names in map file, and query sidewalk name from nearest road.
177kb savings in Greater London. not much but it's something.

These sidewalks have no name tagged in OSM.

https://github.com/user-attachments/assets/bee56282-8d2c-4fb7-a0ac-14d8006a9595

supersedes organicmaps/organicmaps#8861

Don't store sidewalk names in map file, and query sidewalk name from nearest road. 177kb savings in Greater London. not much but it's something. These sidewalks have no name tagged in OSM. https://github.com/user-attachments/assets/bee56282-8d2c-4fb7-a0ac-14d8006a9595 supersedes https://git.omaps.dev/organicmaps/organicmaps/pulls/8861

🚀 2

flomnes (Migrated from github.com) reviewed 2024-09-01 12:05:27 +00:00

indexer/ftypes_matcher.cpp

					
				@ -714,1 +714,4 @@

				IsSidewalkChecker::IsSidewalkChecker() : BaseChecker(3 /* level */)

				{

				  m_types.push_back(classif().GetTypeByPath({"highway", "footway", "sidewalk"}));

flomnes (Migrated from github.com) commented

2024-09-01 11:50:15 +00:00

I'm not sure why highway is present here, I wouldn't walk in one but hey, you do you

I'm not sure why **highway** is present here, I wouldn't walk in one but hey, you do you

map/framework.cpp

					
				@ -613,0 +615,4 @@

				  auto const & dataSource = m_featuresFetcher.GetDataSource();

				  search::ReverseGeocoder const coder(dataSource);

				  MwmSet::MwmId const mwmId = dataSource.GetMwmIdByCountryFile(platform::CountryFile(m_infoGetter->GetRegionCountryId(pt)));

				  return coder.GetNearbyStreets(mwmId, pt)[0];

flomnes (Migrated from github.com) commented

2024-09-01 11:45:47 +00:00

Empty coder.GetNearbyStreets(mwmId, pt) may not be handled properly.

Empty `coder.GetNearbyStreets(mwmId, pt)` may not be handled properly.

map/place_page_info.cpp

flomnes (Migrated from github.com) commented

2024-09-01 12:03:21 +00:00

This function is getting a bit complex, I would recommend splitting it into smaller functions (one for buildings, one for sidewalks, etc.).

Using free functions would allow to easily see side effects of each function, e.g

if (IsBookmark())
  std::tie(m_uiTitle, m_uiSecondaryTitle) = handleBookmark(/* ... */);
else if (IsBuilding())
  std::tie(m_uiTitle, m_uiAddress) = handleBuilding(/* ... */);
else if(IsSidewalk())
  std::tie(m_uiTitle, m_uiAddress) = handleSidewalk(/* ... */);

This function is getting a bit complex, I would recommend splitting it into smaller functions (one for buildings, one for sidewalks, etc.). Using free functions would allow to easily see side effects of each function, e.g ```cpp if (IsBookmark()) std::tie(m_uiTitle, m_uiSecondaryTitle) = handleBookmark(/* ... */); else if (IsBuilding()) std::tie(m_uiTitle, m_uiAddress) = handleBuilding(/* ... */); else if(IsSidewalk()) std::tie(m_uiTitle, m_uiAddress) = handleSidewalk(/* ... */); ```

map/place_page_info.cpp

flomnes (Migrated from github.com) commented

2024-09-01 11:43:50 +00:00

    search::ReverseGeocoder::Street nearestStreet = framework-> GetNearestStreet(m_buildInfo.m_mercator);

```suggestion search::ReverseGeocoder::Street nearestStreet = framework-> GetNearestStreet(m_buildInfo.m_mercator); ```

biodranik (Migrated from github.com) reviewed 2024-09-01 16:42:21 +00:00

map/place_page_info.cpp

biodranik (Migrated from github.com) commented

2024-09-01 16:42:21 +00:00

Is std::tie needed with C++17 and above?

Will auto const & [m_uiTitle, m_uiSecondaryTitle] = handleBookmark(/* ... */); work?

Is std::tie needed with C++17 and above? Will `auto const & [m_uiTitle, m_uiSecondaryTitle] = handleBookmark(/* ... */);` work?

flomnes (Migrated from github.com) reviewed 2024-09-01 17:41:31 +00:00

map/place_page_info.cpp

flomnes (Migrated from github.com) commented

2024-09-01 17:41:31 +00:00

No, because m_uiTitle and m_uiSecondaryTitle are already declared (as class data members in this example).

No, because `m_uiTitle` and `m_uiSecondaryTitle` are already declared (as class data members in this example).

biodranik (Migrated from github.com) reviewed 2024-09-01 17:43:27 +00:00

map/place_page_info.cpp

biodranik (Migrated from github.com) commented

2024-09-01 17:43:27 +00:00

Right, we're working with local variables 🤦‍♂️

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

No reviewers

No labels

Android Automotive (AAOS)

API

AppGallery

AppStore

Battery and Performance

External Map Datasets

F-Droid

Fonts

Frequently User Reported

No milestone

No project

No assignees

2 participants

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#8975

Rows
Columns