Display housenumbers above icons and main captions #5700

pastk · 2023-08-11T15:37:04Z

pastk commented

2023-08-11 15:37:04 +00:00

A new approach is to display housenumbers above icons, see organicmaps/organicmaps#5700 (comment)

An original old post with an attempt to use secondary captions:

If a secondary caption is not used then it's filled with a house number.

No change for cases when the secondary caption is used for int_name display.

In the vehicle style housenumbers always replace the primary caption, so the behavior didn't change there.

A new approach is to display housenumbers above icons, see https://git.omaps.dev/organicmaps/organicmaps/pulls/5700#issuecomment-1713965203 ### An original old post with an attempt to use secondary captions: If a secondary caption is not used then it's filled with a house number. ![image](https://github.com/organicmaps/organicmaps/assets/18434508/d4a5aece-65ab-4e39-a3d8-9b36cb92b9c4) No change for cases when the secondary caption is used for `int_name` display. In the vehicle style housenumbers always replace the primary caption, so the behavior didn't change there.

biodranik (Migrated from github.com) reviewed 2023-08-11 15:37:04 +00:00

biodranik (Migrated from github.com) reviewed 2023-08-11 23:12:59 +00:00

biodranik (Migrated from github.com) left a comment

Secondary house number looks good.
Please do not suppress anything.
Test it in the UK. You may grep areas with number + name via overpass turbo.
@vng can implementation be better?

1. Secondary house number looks good. 2. Please do not suppress anything. 3. Test it in the UK. You may grep areas with number + name via overpass turbo. 4. @vng can implementation be better?

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-08-11 23:07:34 +00:00

What are the benefits of making them always optional? What are the cons?

drape_frontend/stylist.cpp

					
				@ -470,1 +427,3 @@

				          aggregator.m_rules.push_back({ dRule, static_cast<float>(addressKeys[0].m_priority), false });

				          TRuleWrapper const hnRule({ dRule, static_cast<float>(addressKeys[0].m_priority),

				                                      false, true /* m_isHouseNumber*/ });

				          aggregator.m_rules.push_back(hnRule);

biodranik (Migrated from github.com) commented

2023-08-11 23:10:38 +00:00

Won't it slow down the rendering?
Cache bools.

1. Won't it slow down the rendering? 2. Cache bools.

pastk reviewed 2023-08-12 07:23:11 +00:00

drape_frontend/stylist.cpp

					
				@ -470,1 +427,3 @@

				          aggregator.m_rules.push_back({ dRule, static_cast<float>(addressKeys[0].m_priority), false });

				          TRuleWrapper const hnRule({ dRule, static_cast<float>(addressKeys[0].m_priority),

				                                      false, true /* m_isHouseNumber*/ });

				          aggregator.m_rules.push_back(hnRule);

pastk commented

2023-08-12 07:23:11 +00:00

They're calculated once. Its just getting a stored bool value from the object.

pastk reviewed 2023-08-12 07:27:01 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-08-12 07:27:00 +00:00

I see no cons.

Even in the only case of a mandatory caption plus a star icon for capitals - if e.g. a country name displaces the secondary int_name of the capital then the primary name and a star icon better stay, right?

Other cases are like a train station with a secondary int_name. Also better to displace just the secondary one and let the primary stay instead of removing them both at once.

I see no cons. Even in the only case of a mandatory caption plus a star icon for capitals - if e.g. a country name displaces the secondary int_name of the capital then the primary name and a star icon better stay, right? Other cases are like a train station with a secondary int_name. Also better to displace just the secondary one and let the primary stay instead of removing them both at once.

vng (Migrated from github.com) reviewed 2023-08-15 13:41:46 +00:00

drape_frontend/apply_feature_functors.cpp

vng (Migrated from github.com) commented

2023-08-15 13:37:20 +00:00

Should we assign else here?

else
  textParams.m_specialDisplacement = None;

Should we assign else here? ``` else textParams.m_specialDisplacement = None; ```

drape_frontend/stylist.cpp

vng (Migrated from github.com) commented

2023-08-15 13:33:45 +00:00

I suspect that correct braces should be like:

if (mainTextType == drule::text_type_housenumber ||
   (mainTextType == drule::text_type_name && (m_mainText.empty() || m_houseNumber.find(m_mainText) != std::string::npos)))

I suspect that correct braces should be like: ``` if (mainTextType == drule::text_type_housenumber || (mainTextType == drule::text_type_name && (m_mainText.empty() || m_houseNumber.find(m_mainText) != std::string::npos))) ```

drape_frontend/stylist.cpp

vng (Migrated from github.com) commented

2023-08-15 13:35:53 +00:00

If we don't care what will be left in m_houseNumber, better to write here and below:

m_mainText = std::move(m_houseNumber);

If we don't care what will be left in m_houseNumber, better to write here and below: ``` m_mainText = std::move(m_houseNumber); ```

generator/generator_tests/feature_builder_test.cpp

vng (Migrated from github.com) commented

2023-08-15 13:41:40 +00:00

Please, remove your changes here, oms2type, feature_data.
This logic is reworked in generator branch and I will get unnecessary merge conflicts.
It will be in a PR soon.

Anyway, what is the reason to check 2 sequential AddHouseName calls? It won't happen in the generator anyway.

Please, remove your changes here, oms2type, feature_data. This logic is reworked in generator branch and I will get unnecessary merge conflicts. It will be in a PR soon. Anyway, what is the reason to check 2 sequential AddHouseName calls? It won't happen in the generator anyway.

vng (Migrated from github.com) reviewed 2023-09-17 22:18:57 +00:00

drape_frontend/apply_feature_functors.cpp

vng (Migrated from github.com) commented

2023-09-17 21:58:48 +00:00

Why do we test hasIcon above if we have ASSERT(!hasIcon) here ?

drape_frontend/stylist.cpp

					
				@ -116,3 +113,4 @@

				    , m_captionRule({ nullptr, 0, false, false })

				    , m_f(f)

				    , m_geomType(type)

				    , m_zoomLevel(zoomLevel)

vng (Migrated from github.com) commented

2023-09-17 21:48:38 +00:00

Does m_captionRule != nullptr means m_mainCaptionFound == true ?
Also I think that m_auxCaptionFound flag also not needed.
Can replace with TRuleWrapper::HasMainCaption, TRuleWrapper::HasAuxCaption

Does m_captionRule != nullptr means m_mainCaptionFound == true ? Also I think that m_auxCaptionFound flag also not needed. Can replace with TRuleWrapper::HasMainCaption, TRuleWrapper::HasAuxCaption

drape_frontend/stylist.cpp

					
				@ -269,3 +262,3 @@

				                              bool const auxCaptionExists)

				                              feature::GeomType const type, bool const auxCaptionExists)

				{

				  feature::NameParamsOut out;

vng (Migrated from github.com) commented

2023-09-17 22:05:07 +00:00

ASSERT(m_auxText.empty() || !m_mainText.empty())

drape_frontend/stylist.cpp

					
				@ -437,2 +392,3 @@

				    if (s.m_captionDescriptor.IsNameExists())

				      aggregator.m_rules.push_back(aggregator.m_captionRule);

				    else

vng (Migrated from github.com) commented

2023-09-17 22:15:09 +00:00

Looks like a good optimization. Better not to hardcode, but take out for example into RulesHolder and calculate this scale once. Like RulesHolder::GetMinHNZoom().

pastk reviewed 2023-09-18 12:41:15 +00:00

drape_frontend/stylist.cpp

					
				@ -437,2 +392,3 @@

				    if (s.m_captionDescriptor.IsNameExists())

				      aggregator.m_rules.push_back(aggregator.m_captionRule);

				    else

pastk commented

2023-09-18 12:41:15 +00:00

Yes, it'll be more flexible for sure.
But it'll also pollute RulesHolder loading logic with the housenumbers exception. So instead of a single line hardcoded housenumbers zoom level we'll have many hardcoded lines for a way to get this housenumbers zoom level...
And we have a hardcoded z18- min zoom for point housenumbers anyway....
So I just left a TODO for now.

Yes, it'll be more flexible for sure. But it'll also pollute RulesHolder loading logic with the housenumbers exception. So instead of a single line hardcoded housenumbers zoom level we'll have many hardcoded lines for a way to get this housenumbers zoom level... And we have a hardcoded z18- min zoom for point housenumbers anyway.... So I just left a TODO for now.

pastk reviewed 2023-09-18 12:41:22 +00:00

drape_frontend/stylist.cpp

					
				@ -269,3 +262,3 @@

				                              bool const auxCaptionExists)

				                              feature::GeomType const type, bool const auxCaptionExists)

				{

				  feature::NameParamsOut out;

pastk commented

2023-09-18 12:41:22 +00:00

fixed, thanks!

pastk reviewed 2023-09-18 12:41:27 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-09-18 12:41:27 +00:00

A condition above is for (hasIcon || hasHouseNumber).
An assert is for icons only to catch bugs (they're being fixed here in runtime anyway, but good to fix in the styles). We can change the logic here once we're sure all related bugs in styles had been fixed (or once we decide what to do with text-offset: style params).

A condition above is for (hasIcon || hasHouseNumber). An assert is for icons only to catch bugs (they're being fixed here in runtime anyway, but good to fix in the styles). We can change the logic here once we're sure all related bugs in styles had been fixed (or once we decide what to do with `text-offset:` style params).

pastk reviewed 2023-09-18 12:41:31 +00:00

drape_frontend/stylist.cpp

					
				@ -116,3 +113,4 @@

				    , m_captionRule({ nullptr, 0, false, false })

				    , m_f(f)

				    , m_geomType(type)

				    , m_zoomLevel(zoomLevel)

pastk commented

2023-09-18 12:41:31 +00:00

good catch, thanks!

vng (Migrated from github.com) reviewed 2023-09-18 18:00:08 +00:00

drape_frontend/apply_feature_functors.cpp

vng (Migrated from github.com) commented

2023-09-18 18:00:08 +00:00

Ok, rewrite like:

if (m_textParams.m_titleDecl.m_anchor == dp::Anchor::Center)
{
  ASSERT(!hasIcon, (m_id));
  if (hasHouseNumber)
    m_textParams.m_titleDecl.m_anchor = GetAnchor(0, 1);
}

Ok, rewrite like: ``` if (m_textParams.m_titleDecl.m_anchor == dp::Anchor::Center) { ASSERT(!hasIcon, (m_id)); if (hasHouseNumber) m_textParams.m_titleDecl.m_anchor = GetAnchor(0, 1); } ```

vng (Migrated from github.com) approved these changes 2023-09-18 18:07:00 +00:00

pastk reviewed 2023-09-18 18:41:40 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-09-18 18:41:40 +00:00

Still, we'd want to fix the anchor for the icon case, so the second if would be if (hasHouseNumber || hasIcon)

Still, we'd want to fix the anchor for the icon case, so the second `if` would be `if (hasHouseNumber || hasIcon)`

pastk reviewed 2023-09-18 18:43:52 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-09-18 18:43:52 +00:00

So I don't know if this code will be any better, conditions will be the same, but just split into two lines..

vng (Migrated from github.com) reviewed 2023-09-19 00:18:03 +00:00

drape_frontend/apply_feature_functors.cpp

vng (Migrated from github.com) commented

2023-09-19 00:18:03 +00:00

Still, we'd want to fix the anchor for the icon case, so the second if would be if (hasHouseNumber || hasIcon)

No because ASSERT(!hasIcon). Or I incorrectly interpret your code?

I try to avoid misambiguity. We have:
if (X || ...)
{
ASSERT(!X, ()); // so if we have !X here, why do we check if (X) above?
}

> Still, we'd want to fix the anchor for the icon case, so the second if would be if (hasHouseNumber || hasIcon) No because ASSERT(!hasIcon). Or I incorrectly interpret your code? I try to avoid misambiguity. We have: if (X || ...) { ASSERT(!X, ()); // so if we have !X here, why do we check if (X) above? }

pastk reviewed 2023-09-19 10:01:27 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-09-19 10:01:26 +00:00

There could be cases in prod when there is icon and anchor is set to center.
We want to fix them at runtime and at the same time fire an assert in debug to catch those cases and fix in styles.

There could be cases in prod when there is icon and anchor is set to center. We want to fix them at runtime and at the same time fire an assert in debug to catch those cases and fix in styles.

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

No reviewers

biodranik

vng