organicmaps/organicmaps-tmp

Archived

[drape] Refactor Stylist and RuleDrawer #6034

Merged

root merged 1 commit from pastk-drape-stylist into master

2023-09-28 16:31:46 +00:00

pastk commented

2023-09-14 21:30:11 +00:00

Member

Depends on housenumbers PRs, so please check the last commit only!

Simplify the structure, reduce same parameters passing and storing several times, better division of classes' responsibilities, many smaller optimizations (like don't do various pre-processing if there are no drules that will use it), get rid of TRuleWrapper...

I plan to continue refactoring the related code in subsequent PRs:

optimize drules filtering in Stylist and get rid of Key.m_hatching
simplify apply_feature_functors and its communication with the RuleDrawer

Depends on housenumbers PRs, so please check the last commit only! Simplify the structure, reduce same parameters passing and storing several times, better division of classes' responsibilities, many smaller optimizations (like don't do various pre-processing if there are no drules that will use it), get rid of TRuleWrapper... I plan to continue refactoring the related code in subsequent PRs: - optimize drules filtering in Stylist and get rid of Key.m_hatching - simplify apply_feature_functors and its communication with the RuleDrawer

biodranik (Migrated from github.com) reviewed 2023-09-14 21:30:11 +00:00

AndrewShkrob reviewed 2023-09-14 21:30:11 +00:00

vng (Migrated from github.com) reviewed 2023-09-25 16:45:00 +00:00

drape_frontend/apply_feature_functors.cpp

vng (Migrated from github.com) commented

2023-09-25 16:19:47 +00:00

nit: It looks very safe, but we usually don't write const for integral types in input function params by value.
Unusual syntax overhead imo.

nit: It looks very safe, but we usually don't write const for integral types in input function params by value. Unusual syntax overhead imo.

vng (Migrated from github.com) commented

2023-09-25 16:20:33 +00:00

How NAN is possible in theory?

How NAN is possible in theory?

vng (Migrated from github.com) commented

2023-09-25 16:23:43 +00:00

nit: one-line without {}

nit: one-line without {}

drape_frontend/apply_feature_functors.cpp

					
				@ -704,19 +739,20 @@ void ApplyAreaFeature::CalculateBuildingOutline(bool calculateNormals, BuildingO

				  }

				}

vng (Migrated from github.com) commented

2023-09-25 16:25:37 +00:00

nit: Same here for const

nit: Same here for const

drape_frontend/rule_drawer.cpp

vng (Migrated from github.com) commented

2023-09-25 16:30:13 +00:00

Aha, here it is :) I'd prefer simple -1 value and check that areaDepth > 0 in a Priprity function.

Aha, here it is :) I'd prefer simple -1 value and check that areaDepth > 0 in a Priprity function.

drape_frontend/rule_drawer.cpp

					
				@ -385,1 +381,3 @@

				    s.ForEachRule(std::bind(&ApplyLineFeatureAdditional::ProcessLineRule, &applyAdditional, _1));

				    ApplyLineFeatureAdditional applyAdditional(m_context->GetTileKey(), insertShape, f, m_currentScaleGtoP,

				                                               s.GetCaptionDescription(), clippedSplines);

				    if (s.m_pathtextRule != nullptr)

vng (Migrated from github.com) commented

2023-09-25 16:30:37 +00:00

Should be >0 right?

Should be >0 right?

indexer/drawing_rule_def.cpp

vng (Migrated from github.com) commented

2023-09-25 16:41:48 +00:00

My fix is missing. Happens that areaSize is 0 -> areaSizeCompact is NAN.

My fix is missing. Happens that areaSize is 0 -> areaSizeCompact is NAN.

pastk reviewed 2023-09-25 17:14:50 +00:00

indexer/drawing_rule_def.cpp

pastk commented

2023-09-25 17:14:50 +00:00

Author

Member

yeap lost in a rebase
I'll fix it.

yeap lost in a rebase I'll fix it.

pastk reviewed 2023-09-25 17:16:11 +00:00

drape_frontend/rule_drawer.cpp

pastk commented

2023-09-25 17:16:11 +00:00

Author

Member

areaDepth should be in the [-11000,-10100) range, but we might move/resize the ranges, so using NAN is more reliable.

areaDepth should be in the `[-11000,-10100)` range, but we might move/resize the ranges, so using NAN is more reliable.

pastk reviewed 2023-09-25 17:17:45 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-09-25 17:17:45 +00:00

Author

Member

fixed

fixed

pastk reviewed 2023-09-25 17:18:26 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-09-25 17:18:26 +00:00

Author

Member

fixed

fixed

pastk reviewed 2023-09-25 17:19:21 +00:00

drape_frontend/apply_feature_functors.cpp

					
				@ -704,19 +739,20 @@ void ApplyAreaFeature::CalculateBuildingOutline(bool calculateNormals, BuildingO

				  }

				}

pastk commented

2023-09-25 17:19:21 +00:00

Author

Member

fixed

fixed

biodranik (Migrated from github.com) reviewed 2023-09-25 18:18:57 +00:00

drape_frontend/rule_drawer.cpp

biodranik (Migrated from github.com) commented

2023-09-25 18:18:57 +00:00

Please do not use NAN. We use -ffast-math in our code.

Please do not use NAN. We use [-ffast-math](https://stackoverflow.com/a/22135559) in our code.

vng (Migrated from github.com) reviewed 2023-09-25 19:40:11 +00:00

drape_frontend/rule_drawer.cpp

vng (Migrated from github.com) commented

2023-09-25 19:40:10 +00:00

Yes, add kEmptyDepth constant somewhere near kBaseDepthXXX.

Yes, add kEmptyDepth constant somewhere near kBaseDepthXXX.

pastk reviewed 2023-09-26 18:26:08 +00:00

drape_frontend/rule_drawer.cpp

pastk commented

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

Author

Member

Changed to an out-of-range const.
PTAL!

Changed to an out-of-range const. PTAL!

vng (Migrated from github.com) approved these changes 2023-09-26 19:21:21 +00:00

drape_frontend/apply_feature_functors.cpp

vng (Migrated from github.com) commented

2023-09-26 19:14:00 +00:00

nit: remove const

nit: remove const

drape_frontend/apply_feature_functors.cpp

vng (Migrated from github.com) commented

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

nit: remove const near bool

nit: remove const near bool

drape_frontend/apply_feature_functors.cpp

vng (Migrated from github.com) commented

2023-09-26 19:15:14 +00:00

nit: Here can add double const depth

nit: Here can add double const depth

drape_frontend/stylist.cpp

					
				@ -262,13 +40,13 @@ bool IsHatchingTerritoryChecker::IsMatched(uint32_t type) const

				  return std::find(iEnd3, m_types.end(), PrepareToMatch(type, 2)) != m_types.end();

vng (Migrated from github.com) commented

2023-09-26 19:19:47 +00:00

nit: Remove consts here

nit: Remove consts here

drape_frontend/stylist.cpp

vng (Migrated from github.com) commented

2023-09-26 19:20:19 +00:00

nit: remove const

nit: remove const

drape_frontend/stylist.hpp

vng (Migrated from github.com) commented

2023-09-26 19:20:54 +00:00

nit: remove consts here

nit: remove consts here

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

3 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#6034

No description provided.