Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Advanced label formatting using ["text-section"] expression #13904

Closed
wants to merge 8 commits into from
Closed

[core] Advanced label formatting using ["text-section"] expression #13904

wants to merge 8 commits into from

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented Feb 11, 2019 •
edited
Loading

This PR introduces ["text-section"] expression that is evaluated to section id when symbol layer is laid out. It allows to use decision expressions to define section specific paint properties, for example:

"layout": {
  "text-field": ["format",
                  ["get", "name_en"], { "id": "header" },
                  "\n", {},
                  ["get", "description_en"], { "id": "description" }
                ],
},
"paint": {
  "text-color": ["match", 
                  ["text-section"],
                    "header", "hsl(35, 8%, 85%)",
                    "description", "#0F9D58",
                    "black"
                ]
}

Symbol formatting using multiple sections. Section id assigned to each character.

adv_label_format

TODO

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

Fixes: #13813

/cc @asheemmamoowala @ryanhamley

@alexshalamov alexshalamov added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Feb 11, 2019
@alexshalamov
Copy link
Contributor Author

@asheemmamoowala @ryanhamley I'm trying to implement something similar in gl-js, to see if proposed solution is acceptable. Could you please comment if you see any problems with proposed solution?

@alexshalamov alexshalamov mentioned this pull request Feb 14, 2019
Closed
5 tasks
@alexshalamov alexshalamov force-pushed the alexshalamov_text_section_expr branch from 6e9f7f2 to 324ba40 Compare February 19, 2019 15:05
@alexshalamov
[core] Add ["text-section"] expression
c7c4f99 
New expression evaluates to formatted section id (string | number) that
can be used within decision expressions, such as 'match' or 'case', so
that each formatted section can have different paint properties.
@alexshalamov
[core] Unit test for text-section compound expression
f72b2ad
@alexshalamov alexshalamov force-pushed the alexshalamov_text_section_expr branch from 324ba40 to d02e1d1 Compare March 4, 2019 08:02
@alexshalamov alexshalamov force-pushed the alexshalamov_text_section_expr branch from d02e1d1 to b1fcbd5 Compare March 4, 2019 08:22
@alexshalamov
Copy link
Contributor Author

Benchmark result for rendering single frame (Manhattan, streets v7, zoom level 15). Formatted labels style has single formatted section and text-color paint property uses match expression with ["text-section"].

---------------------------------------------------------------------------------------
Benchmark                                                Time           CPU Iterations
---------------------------------------------------------------------------------------
API_renderStill_reuse_map_mean                    10372558 ns   10114118 ns      53
API_renderStill_reuse_map_median                  10327769 ns   10078607 ns      53
API_renderStill_reuse_map_formatted_labels_mean   10944942 ns   10767448 ns      50
API_renderStill_reuse_map_formatted_labels_median 10961336 ns   10788774 ns      50 +0.7ms

@alexshalamov alexshalamov requested a review from pozdnyakov March 4, 2019 08:40
@alexshalamov alexshalamov changed the title [wip][core] Advanced label formatting using ["text-section"] expression [core] Advanced label formatting using ["text-section"] expression Mar 4, 2019
@alexshalamov alexshalamov requested a review from kkaefer March 4, 2019 08:40
pozdnyakov
pozdnyakov reviewed Mar 4, 2019
View reviewed changes
include/mbgl/style/expression/formatted.hpp Show resolved Hide resolved
include/mbgl/style/expression/expression.hpp Outdated Show resolved Hide resolved
include/mbgl/style/expression/formatted.hpp Outdated Show resolved Hide resolved
include/mbgl/style/property_expression.hpp Outdated
class PropertyExpression final : public PropertyExpressionBase {
public:
// Second parameter to be used only for conversions from legacy functions.
PropertyExpression(std::unique_ptr<expression::Expression> expression_, optional<T> defaultValue_ = {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: = nullopt

include/mbgl/style/property_expression.hpp
const expression::EvaluationResult result = expression->evaluate(expression::EvaluationContext({zoom}, &feature));
T evaluate(const expression::EvaluationContext& context, T finalDefaultValue = T()) const {
assert(canEvaluateWith(context));
const expression::EvaluationResult result = expression->evaluate(context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const expression::EvaluationResult&

src/mbgl/text/tagged_string.hpp Outdated Show resolved Hide resolved
src/mbgl/layout/symbol_layout.cpp Outdated
return (*options.id).match(
[] (double t) -> expression::Value { return t; },
[] (const std::string& t) -> expression::Value { return t; },
[] (auto&) -> expression::Value { return {}; });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto&

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we assert here?

src/mbgl/layout/symbol_layout.cpp Outdated Show resolved Hide resolved
src/mbgl/layout/symbol_layout.cpp Outdated
}

if (symbolInstance.writingModes & WritingModeType::Vertical) {
const Range<float> sizeData = bucket->textSizeBinder->getVertexSizeData(feature);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const Range& sizeData

src/mbgl/layout/symbol_layout.cpp Outdated Show resolved Hide resolved
pozdnyakov
pozdnyakov approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@LukasPaczos LukasPaczos mentioned this pull request Mar 5, 2019
Closed
@alexshalamov alexshalamov mentioned this pull request Mar 7, 2019
Merged
6 tasks
@alexshalamov
Copy link
Contributor Author

Closing this, rewritten PR can be found here: #14062

@alexshalamov alexshalamov deleted the alexshalamov_text_section_expr branch March 15, 2019 16:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pozdnyakov pozdnyakov pozdnyakov approved these changes

@kkaefer kkaefer Awaiting requested review from kkaefer

Assignees
No one assigned
Labels
⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexshalamov @pozdnyakov