[drape] Refactor apply_feature_functors #6243

pastk · 2023-10-06T10:54:45Z

pastk commented

2023-10-06 10:54:45 +00:00

Follow-up to #6034.

Optimizations:

don't parse a road ref if there is no shield drule
don't process a shield drule if there are no road shields in a ref
call ClipSplineByRect() once per linear feature (not for each line drule)
calculate areaDepth for BG-by-size areas only

...and many minor things like replace redundant checks with asserts, less params wrapping and passing, simpler structure, etc.

P.S. I'll squash and sign commits after review.

Follow-up to #6034. Optimizations: - don't parse a road ref if there is no shield drule - don't process a shield drule if there are no road shields in a ref - call ClipSplineByRect() once per linear feature (not for each line drule) - calculate areaDepth for BG-by-size areas only ...and many minor things like replace redundant checks with asserts, less params wrapping and passing, simpler structure, etc. P.S. I'll squash and sign commits after review.

vng (Migrated from github.com) reviewed 2023-10-08 13:13:26 +00:00

drape_frontend/apply_feature_functors.hpp

vng (Migrated from github.com) commented

2023-10-08 13:12:21 +00:00

I'd prefer to keep here and fix in other functors best (double) precision for m_currentScaleGtoP.
Also, make m_baseGtoPScale -> double (LineViewParams, AreaViewParams). Because later they are used as double. I suspect that may have an impact in points conversion. float plays only after the final points conversion.

I'd prefer to keep here and fix in other functors best (double) precision for m_currentScaleGtoP. Also, make m_baseGtoPScale -> double (LineViewParams, AreaViewParams). Because later they are used as double. I suspect that may have an impact in points conversion. float plays only after the final points conversion.

pastk reviewed 2023-10-08 13:18:39 +00:00

drape_frontend/apply_feature_functors.hpp

pastk commented

2023-10-08 13:18:39 +00:00

Ok, I'll do!

pastk reviewed 2023-10-08 16:46:08 +00:00

drape_frontend/apply_feature_functors.hpp

pastk commented

2023-10-08 16:46:08 +00:00

actually it seems like in LineShape its used as float only

vng (Migrated from github.com) approved these changes 2023-10-08 17:06:46 +00:00

vng (Migrated from github.com) left a comment

LGTM. Can't guarantee that I've checked all possible cases :)
All changes looks reasonable for me. Should test on devices.

LGTM. Can't guarantee that I've checked all possible cases :) All changes looks reasonable for me. Should test on devices.

drape_frontend/line_shape.cpp

vng (Migrated from github.com) commented

2023-10-08 16:53:10 +00:00

Ok, good to convert here.

biodranik (Migrated from github.com) reviewed 2023-10-08 21:07:01 +00:00

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-10-08 18:30:49 +00:00

Why was this assert removed?

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-10-08 18:31:58 +00:00

An explanation "why" it should not be here in the assert message would be helpful.

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-10-08 18:33:07 +00:00

indentation?

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-10-08 20:47:00 +00:00

  // TODO(KP): degenerate triangles filtering should be done in the generator.

and other TODOs too.

```suggestion // TODO(KP): degenerate triangles filtering should be done in the generator. ``` and other TODOs too.

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-10-08 20:53:39 +00:00

nit: refactor spline to set it in constructor's initializer list, like m_spline{std::make_shared<m2::Spline>(f.GetPointsCount())}

nit: refactor spline to set it in constructor's initializer list, like `m_spline{std::make_shared<m2::Spline>(f.GetPointsCount())}`

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-10-08 20:55:35 +00:00

Why the spline was created in operator() before? Is there a chance that operator() won't be called at all?
Is shared_ptr for spline really needed here? Can we use m2::Spline object member and avoid memory allocation/deallocation?

1. Why the spline was created in operator() before? Is there a chance that operator() won't be called at all? 2. Is shared_ptr for spline really needed here? Can we use m2::Spline object member and avoid memory allocation/deallocation?

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-10-08 20:56:57 +00:00

Will compiler or IDE help us to know if we forgot to initialize/set some variable in all places?

drape_frontend/apply_feature_functors.hpp

biodranik (Migrated from github.com) commented

2023-10-08 20:58:46 +00:00

The code doesn't check areaRule for null. Should it be a reference?

drape_frontend/rule_drawer.cpp

biodranik (Migrated from github.com) commented

2023-10-08 21:02:58 +00:00

What is this case? How often does it happen? Should the higher level code filter it out?

drape_frontend/rule_drawer.cpp

biodranik (Migrated from github.com) commented

2023-10-08 21:02:01 +00:00

Can you reuse these vars in the if above to avoid double checking of the same flags?

drape_frontend/rule_drawer.cpp

biodranik (Migrated from github.com) commented

2023-10-08 21:03:21 +00:00

Why is it not needed anymore?

drape_frontend/visual_params.hpp

					
				@ -28,0 +25,4 @@

				  static double constexpr k6plusScale = 2.4;

				  static double constexpr kXxhdpiScale = 3.0;

				  static double constexpr kXxxhdpiScale = 3.5;

biodranik (Migrated from github.com) commented

2023-10-08 21:04:38 +00:00

There is no need to use static with constexpr.

There is no need to use `static` with constexpr.

drape_frontend/visual_params.hpp

biodranik (Migrated from github.com) commented

2023-10-08 21:05:29 +00:00

Why floats are assigned to doubles?

indexer/drawing_rules.cpp

biodranik (Migrated from github.com) commented

2023-10-08 21:06:13 +00:00

Why check and not assert?

indexer/drawing_rules.cpp

biodranik (Migrated from github.com) commented

2023-10-08 21:06:45 +00:00

Should it be ASSERT?

pastk reviewed 2023-10-10 09:16:45 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-10-10 09:16:45 +00:00

it was moved to drules loading in drawing_rules.cpp

pastk reviewed 2023-10-10 09:17:19 +00:00

indexer/drawing_rules.cpp

pastk commented

2023-10-10 09:17:19 +00:00

fixed,thanks!

pastk reviewed 2023-10-10 09:17:56 +00:00

indexer/drawing_rules.cpp

pastk commented

2023-10-10 09:17:56 +00:00

fixed, thanks!

pastk reviewed 2023-10-10 09:20:30 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-10-10 09:20:29 +00:00

fixed, thanks!

pastk reviewed 2023-10-10 09:20:52 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-10-10 09:20:52 +00:00

fixed

pastk reviewed 2023-10-10 09:27:46 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-10-10 09:27:46 +00:00

fixed, thanks

pastk reviewed 2023-10-10 09:29:12 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-10-10 09:29:12 +00:00

nope, they're init to defaults in shape_view_params.hpp...

pastk reviewed 2023-10-10 09:30:53 +00:00

drape_frontend/rule_drawer.cpp

pastk commented

2023-10-10 09:30:53 +00:00

nope, this code is under #ifdef DEBUG

nope, this code is under `#ifdef DEBUG`

pastk reviewed 2023-10-10 09:39:13 +00:00

drape_frontend/rule_drawer.cpp

pastk commented

2023-10-10 09:39:13 +00:00

its being validated in kothic already

pastk reviewed 2023-10-10 09:43:06 +00:00

drape_frontend/visual_params.hpp

pastk commented

2023-10-10 09:43:06 +00:00

fixed, thanks!

pastk reviewed 2023-10-10 09:49:30 +00:00

drape_frontend/visual_params.hpp

					
				@ -28,0 +25,4 @@

				  static double constexpr k6plusScale = 2.4;

				  static double constexpr kXxhdpiScale = 3.0;

				  static double constexpr kXxxhdpiScale = 3.5;

pastk commented

2023-10-10 09:49:29 +00:00

GCC complains

src/drape_frontend/visual_params.hpp:22:33: error: non-static data member ‘kMdpiScale’ declared ‘constexpr’
   double constexpr kMdpiScale = 1.0;

GCC complains ``` src/drape_frontend/visual_params.hpp:22:33: error: non-static data member ‘kMdpiScale’ declared ‘constexpr’ double constexpr kMdpiScale = 1.0; ```

pastk reviewed 2023-10-10 09:56:03 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-10-10 09:56:03 +00:00

I prefer not to touch the Spline for now..

pastk reviewed 2023-10-10 09:56:13 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-10-10 09:56:13 +00:00

Shouldn't be possible (only in case there is a line with 0 points, which shouldn't be possible)
IDK, I didn't touch the Spline class

1. Shouldn't be possible (only in case there is a line with 0 points, which shouldn't be possible) 2. IDK, I didn't touch the Spline class

pastk reviewed 2023-10-10 10:01:29 +00:00

drape_frontend/rule_drawer.cpp

pastk commented

2023-10-10 10:01:29 +00:00

It happens when e.g. all feature's drules are not drawable on a given zoom level, normally it means the feature shouldn't be in the visibility index for this zoom. But there could be old mwms, vis changes in styles...
Maybe there are some other cases also.

It happens when e.g. all feature's drules are not drawable on a given zoom level, normally it means the feature shouldn't be in the visibility index for this zoom. But there could be old mwms, vis changes in styles... Maybe there are some other cases also.

pastk reviewed 2023-10-10 10:18:24 +00:00

drape_frontend/apply_feature_functors.hpp

pastk commented

2023-10-10 10:18:24 +00:00

What would be the benefit?
(looks like its more matter of style nowadays? https://stackoverflow.com/questions/6732706/c-passing-arguments-by-reference-and-pointer)
I thought to keep passing as pointers so its the same way across the whole file.

What would be the benefit? (looks like its more matter of style nowadays? https://stackoverflow.com/questions/6732706/c-passing-arguments-by-reference-and-pointer) I thought to keep passing as pointers so its the same way across the whole file.

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

drape_frontend/apply_feature_functors.cpp

biodranik (Migrated from github.com) commented

2023-10-10 12:11:23 +00:00

It's not about the spline class. Is spline recreation here necessary? Each allocation and deallocation slows down the code.

biodranik (Migrated from github.com) reviewed 2023-10-10 12:12:21 +00:00

drape_frontend/apply_feature_functors.hpp

biodranik (Migrated from github.com) commented

2023-10-10 12:12:21 +00:00

The benefit is that it's clear that a non-null object is expected.

pastk reviewed 2023-10-10 12:34:29 +00:00

drape_frontend/apply_feature_functors.cpp

pastk commented

2023-10-10 12:34:28 +00:00

Its not necessary and that is why I've removed it.

pastk reviewed 2023-10-10 14:11:12 +00:00

drape_frontend/apply_feature_functors.hpp

pastk commented

2023-10-10 14:11:12 +00:00

changed to refs

biodranik (Migrated from github.com) reviewed 2023-10-10 20:53:31 +00:00

drape_frontend/visual_params.hpp

					
				@ -28,0 +25,4 @@

				  static double constexpr k6plusScale = 2.4;

				  static double constexpr kXxhdpiScale = 3.0;

				  static double constexpr kXxxhdpiScale = 3.5;