`is-supported-script` expression #6260

ChrisLoer · 2018-03-01T21:57:53Z

This is a potential partial fix to address #5807. The idea is to expose an expression that can do a reasonable job of evaluating whether a given string will render "correctly", so that style authors can express ideas like "show local language label if possible, otherwise fall back to transliterated or other value".

Determining what we can render correctly is necessarily guesswork -- even if we had access to the font metadata (which we don't), we'd have to make individual judgment calls about what constituted "semantically significant" shaping (e.g. even in latin scripts, we can't render ligatures, but we don't consider it broken). There are cases where "legibility" may very greatly between fonts: for example, in Arial Unicode, Lao diacritic characters don't have built-in negative metrics, so without shaping support they'll float in empty space, but in Noto Lao diacritics have negative metrics that make them appear in mostly the right place.

I wasn't expecting it to be so complicated to pass a single boolean value into the evaluation context for expressions, but I'm hoping this PR hooks up the plumbing (via EvaluationParameters) to make it straightforward to add more parameters in the future.

TODO:

Implement tests
Performance evaluation (currently benchmark code doesn't use EvaluationParameters, so it won't show any overhead there is in creating that object vs. using a bare GlobalProperties). Checking the plugin availability should be cheap...
Figure out right name, agonize over adding hard-to-define heuristic behavior to style spec

/cc @jfirebaugh @anandthakker @1ec5 @nickidlugash @lucaswoj

anandthakker · 2018-03-06T20:05:40Z

src/style/evaluation_parameters.js

    constructor(zoom: number, options?: *) {
        this.zoom = zoom;
+        this.isRenderable = isRenderable;


isRenderable can just be defined directly as a method on EvaluationParameters (instead of declaring the isRenderable: property above and assigning to this.isRenderable)

Good point, changed.

anandthakker · 2018-03-06T20:19:53Z

src/style/properties.js

-                value.expression.evaluate({zoom: parameters.zoom + 1.0}),
+                value.expression.evaluate(new EvaluationParameters(parameters.zoom - 1.0, parameters)),
+                value.expression.evaluate(parameters),
+                value.expression.evaluate(new EvaluationParameters(parameters.zoom + 1.0, parameters)),


The second argument to EvaluationParameters here (and two lines above) isn't strictly necessary, because expression evaluation doesn't make use of any properties of EvaluationParameters other than zoom.

Then again, if this fact were to change in the future, omitting the argument would cause a bug wherein whatever new important property would get stripped. 👍 LGTM

ChrisLoer · 2018-03-06T21:11:37Z

https://bl.ocks.org/anonymous/raw/db9747f703256c25323232e3c0fe690c/

anandthakker

@ChrisLoer out of curiosity (and for future reference), how did you decide on the renderable / non-renderable content in the test fixtures?

ChrisLoer · 2018-03-07T18:21:59Z

The "Salaam 39" Arabic text is a nice little test string because it mixes RTL and LTR (for the numerals), and testing that we report "true" for Arabic exercises the part of the logic that checks whether the plugin is loaded (in the test suite, the rtl-text-plugin is always loaded, although it's loaded a little differently from live maps). As for the word, well who doesn't like "peace"?

For the non-renderable, I just grabbed a string that I believe says "devanagari" in devanagari script. Nothing special, I just chose something from the ranges I decided to mark "not renderable".

ChrisLoer · 2018-03-08T18:50:28Z

I renamed is-renderable to is-supported-script after discussion with @lucaswoj @1ec5 @anandthakker and @jfirebaugh.

@jfirebaugh is concerned that "supported script" is not descriptive enough, and proposes as an alternative script-rendering-fidelity which returns an enum (full or none). He also thinks using an enum may allow extensibility in the future. I played around with that syntax in a few expressions, and it felt cumbersome to me -- and also like it over-specified (i.e. "full rendering fidelity" isn't quite accurate for plenty of scripts that we'd still say we "support"). Also, I don't see a clear extensibility use case -- would adding an intermediate state like partial actually be useful for style designers?

In a harfbuzz/complex text shaping future, I think the equivalent to this operator would want to take a fontstack as an argument so it could tell you if the fonts themselves had the information necessary to render the string.

nickidlugash · 2018-03-09T02:28:52Z

@ChrisLoer thanks for working on this 😻

I'm hoping this PR hooks up the plumbing (via EvaluationParameters) to make it straightforward to add more parameters in the future.

Can you clarify what kinds of additional parameters could be added?

The char ranges excluded in charInRenderableScript don't seem to be inclusive of all scripts that need complex shaping, e.g. Khmer. Should we exlcude all the blocks that are part of scripts that have Shaping required: YES here? http://www.unicode.org/repos/cldr/trunk/common/properties/scriptMetadata.txt

(Some scripts have a MIN value for shaping, including Latin and Thai – there may be exceptions but we should not exclude these from being rendered for now, I think).

I'm working right now on reviewing certain scripts (like Thai and Lao) that are most relevant to our core style needs, to get a better practical understanding of how legible these scripts are to native reader (taking into account, as you pointed out, the possible variation caused by different fonts). Would we feel comfortable manually adjusting the char ranges in charInRenderableScript based on these reviews?

would adding an intermediate state like partial actually be useful for style designers?

Perhaps we could use something like partial for scripts that can be rendered correctly/almost correctly in certain fonts? It would be up to users discretion whether they think their font can render these correctly. For our core styles, I think we would strive to support this subset of scripts.

ChrisLoer · 2018-03-09T17:35:16Z

Can you clarify what kinds of additional parameters could be added?

I wasn't thinking of anything specifically related to this functionality. But before this change there wasn't really a way to connect an expression operator to shared code in the rest of the map.

Perhaps we could use something like partial for scripts that can be rendered correctly/almost correctly in certain fonts?

🤔 So maybe I should wait for you to finish reviewing Thai/Lao/etc to see if we classify them as "partial"? If you think we have a clear use for it, I'm happy to go that route. Can I outsource the entire set of classifications to you? I think starting from CLDR like you suggested makes sense -- ideally we can condense that into just a handful of broad ranges that require shaping. For the RTL languages (which are all marked Shaping: YES), we'd need a case-by-case judgment (I know Hebrew and Arabic are "good enough", but actually don't know how legible Syriac is for instance)

nickidlugash · 2018-03-12T22:25:12Z

Can I outsource the entire set of classifications to you?

Sure, I can take a stab.

For clarification, if this operator was an enum rather than a boolean, what would the most efficient syntax look like? Would you need to wrap the operator in an equality operator?

Perhaps we could use something like partial for scripts that can be rendered correctly/almost correctly in certain fonts?

Alternatively, we could also consider just including those scripts as is-supported-script: true if we want to stick to a boolean. If we can create font stacks to correctly display these scripts in our core styles, then we'll also be able to provide users with style guidelines and fonts to do the same.

1ec5 · 2018-03-21T06:21:36Z

I know Hebrew and Arabic are "good enough", but actually don't know how legible Syriac is for instance

These expressions would be used most commonly on map data, so it might be possible to simplify the question of renderability based on OpenStreetMap tagging practices. For example, in mapbox/mapbox-gl-native#6057, I found that a lack of support for Hebrew niqqud marks would’ve been no problem because it would’ve affected only a handful of street-level features.

nickidlugash · 2018-03-27T21:49:15Z

@ChrisLoer per chat, I've updated the unicode ranges to flag, though they are still subject to slight adjustments, since I'm still doing some legibility checks with native readers.

Regarding whether it would be useful to have partial or other more descriptive values, I'm not sure, but I think perhaps a boolean is fine. 🤔 From the standpoint of our default map styling, we only need two conditions: Display the text if the script is "supported", and hide the text if the script is "unsupported" (adding more conditionals could be helpful but would probably not be worth the added complexity to the expression values for text-field assignment. They're already going to be pretty complex).

If we only have boolean values in this operator, then I presume we'll make the judgement call of where that somewhat fuzzy line is based on what we've deemed best for our default styles. If we have more nuanced values, customers would have more flexibility (both with adjusting the display in our default styles, and with custom styles/data), but my sense is that there is not a demand for this. We've always baked certain language display decisions in our vector tiles and default styles, and so far we haven't received any feedback to indicate that customers want the degree of language display control enabled by this operator. I think it will mainly be used in our default styles.

ChrisLoer · 2018-03-28T18:03:40Z

Thanks for those changes @nickidlugash! Sorry the rebase-on-top-of-rollup got a bit messy and I ended up squashing/re-writing you out of the commit history.

@jfirebaugh or @anandthakker, can I get another round of review? @jfirebaugh I know I never convinced you on is-supported-script vs script-rendering-fidelity but from further discussion with @nickidlugash I think a plain boolean is appropriate here. If it helps, I'm happy to grant "I told you so" rights for when this decision comes back to bite us. 😜

jfirebaugh · 2018-03-29T00:07:50Z

src/style-spec/expression/definitions/index.js

+        [StringType],
+        // At parse time this will always return true, so we need to exclude this expression with isGlobalPropertyConstant
+        (ctx, [s]) => {
+            if (ctx.globals && ctx.globals.isSupportedScript) {


const isSupportedScript = ctx.globals && ctx.globals.isSupportedScript; if (isSupportedScript) { return isSupportedScript(s.evaluate(ctx)); }

jfirebaugh · 2018-03-29T00:14:24Z

src/util/script_detection.js

@@ -263,6 +263,44 @@ export function charHasNeutralVerticalOrientation(char: number) {
 * @private
 */
 export function charHasRotatedVerticalOrientation(char: number) {
-    return !(charHasUprightVerticalOrientation(char) ||
-             charHasNeutralVerticalOrientation(char));
+    return !(exports.charHasUprightVerticalOrientation(char) ||


s/exports./ (x3)

Did this accidentally work because it gets transpiled to something where exports.charInSupportedScript actually works? Or are the tests incomplete?

Is there a lint we can turn on to flag the use of exports/module.exports in ES6 modules? (cc @anandthakker)

Oops! 😅 I just ran the render tests in the debugger and can verify the functions are getting called. There's an "exports" object which appears to contain the exported functions:

265 function charHasRotatedVerticalOrientation(char ) { >266 debugger; 267 return !(exports.charHasUprightVerticalOrientation(char) || 268 exports.charHasNeutralVerticalOrientation(char)); debug> repl Press Ctrl + C to leave debug repl > exports { allowsIdeographicBreaking: undefined, allowsVerticalWritingMode: undefined, allowsLetterSpacing: undefined, charAllowsLetterSpacing: undefined, charAllowsIdeographicBreaking: undefined, ... } > exports.charHasUprightVerticalOrientation [Function: charHasUprightVerticalOrientation] > exports.charHasUprightVerticalOrientation(char) true > exports.charHasNeutralVerticalOrientation(char) false > exports.charInSupportedScript(char) true

Hm, good point. We should set env.node to false in our root .eslintrc.

nickidlugash · 2018-04-13T21:40:06Z

@ChrisLoer I made an update to the unicode ranges in this check, based on native reader reviews. I'm still working on some reviews but I feel pretty confident about our current ranges.

- Pass completion callback back to foreground so it can answer whether plugin has loaded - Add `isLoaded()` method that can be checked from foreground or background - Serialize error message w/ `toString()` before passing to callback, since we don't have a serialization for `Error#message`

Heuristically evaluates string as "likely to render correctly" based on an audit of current rendering support.

The expression determines if a string is expected to render legibly, based on the Unicode blocks used by the string and the availability of the RTLTextPlugin. This commit standardizes 'EvaluationParameters` as the way to provide global context (in this case, the 'isSupportedString' function) to expression evaluation.

ChrisLoer · 2018-04-14T00:03:22Z

@anandthakker I know you already approved this once, but it ended up getting touched a fair bit after your review. Would you mind taking another quick look before I merge?

anandthakker · 2018-04-18T00:12:39Z

src/source/worker.js

@@ -132,20 +132,20 @@ export default class Worker {
            this.self.importScripts(params.url);
            callback();
        } catch (e) {
-            callback(e);
+            callback(e.toString());


Is this e.toString() because Errors don't serialize properly?

It's been a while since I wrote this, but yeah, what I remember is that some Errors that get generated deep in the importScripts code don't serialize.

anandthakker · 2018-04-18T00:13:31Z

src/source/worker.js

-                }
+                callback(globalRTLTextPlugin.isLoaded() ?
+                    null :
+                    new Error(`RTL Text Plugin failed to import scripts from ${pluginURL}`));


If we're doing e.toString() below, should we also be doing so here?

We could, but web_worker_transfer's serialize works just fine on an Error constructed out of a plain string.

ChrisLoer added the under development label Mar 1, 2018

ChrisLoer requested review from jfirebaugh and anandthakker March 1, 2018 21:57

anandthakker reviewed Mar 6, 2018

View reviewed changes

anandthakker approved these changes Mar 7, 2018

View reviewed changes

ChrisLoer removed the under development label Mar 8, 2018

ChrisLoer changed the title ~~Implement is-renderable expression~~ is-supported-script expression Mar 28, 2018

ChrisLoer force-pushed the renderable-string branch from 6a94bf4 to 67055cc Compare March 28, 2018 17:58

jfirebaugh reviewed Mar 29, 2018

View reviewed changes

ChrisLoer force-pushed the renderable-string branch from 67055cc to 6bd6c6e Compare March 29, 2018 16:00

anandthakker mentioned this pull request Mar 29, 2018

Disallow references to exports, module.exports #6422

Closed

ChrisLoer and others added 5 commits April 13, 2018 16:37

Add isStringInSupportedScript function.

f3df91f

Heuristically evaluates string as "likely to render correctly" based on an audit of current rendering support.

Add tests for 'is-supported-script'

01b6aa9

remove Lao unicode range from charInSupportedScript check

446cbd8

ChrisLoer force-pushed the renderable-string branch from 700ce2a to 446cbd8 Compare April 14, 2018 00:00

ChrisLoer mentioned this pull request Apr 16, 2018

Port is-supported-script expression to native mapbox/mapbox-gl-native#11693

Closed

anandthakker approved these changes Apr 18, 2018

View reviewed changes

ChrisLoer merged commit 66c0e33 into master Apr 18, 2018

ChrisLoer deleted the renderable-string branch April 18, 2018 00:45

ChrisLoer mentioned this pull request Apr 18, 2018

Add an expressions string lookup operator that returns the script of the string? #5807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`is-supported-script` expression #6260

`is-supported-script` expression #6260

ChrisLoer commented Mar 1, 2018 •

edited

Loading

anandthakker Mar 6, 2018

ChrisLoer Mar 6, 2018

anandthakker Mar 6, 2018

anandthakker Mar 6, 2018

ChrisLoer commented Mar 6, 2018

anandthakker left a comment

ChrisLoer commented Mar 7, 2018

ChrisLoer commented Mar 8, 2018

nickidlugash commented Mar 9, 2018

ChrisLoer commented Mar 9, 2018

nickidlugash commented Mar 12, 2018

1ec5 commented Mar 21, 2018 •

edited

Loading

nickidlugash commented Mar 27, 2018

ChrisLoer commented Mar 28, 2018

jfirebaugh Mar 29, 2018

jfirebaugh Mar 29, 2018

ChrisLoer Mar 29, 2018

anandthakker Mar 29, 2018

anandthakker Mar 29, 2018

nickidlugash commented Apr 13, 2018

ChrisLoer commented Apr 14, 2018

anandthakker Apr 18, 2018

ChrisLoer Apr 18, 2018

anandthakker Apr 18, 2018

ChrisLoer Apr 18, 2018

is-supported-script expression #6260

is-supported-script expression #6260

Conversation

ChrisLoer commented Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisLoer commented Mar 6, 2018

anandthakker left a comment

Choose a reason for hiding this comment

ChrisLoer commented Mar 7, 2018

ChrisLoer commented Mar 8, 2018

nickidlugash commented Mar 9, 2018

ChrisLoer commented Mar 9, 2018

nickidlugash commented Mar 12, 2018

1ec5 commented Mar 21, 2018 • edited Loading

nickidlugash commented Mar 27, 2018

ChrisLoer commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickidlugash commented Apr 13, 2018

ChrisLoer commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`is-supported-script` expression #6260

`is-supported-script` expression #6260

ChrisLoer commented Mar 1, 2018 •

edited

Loading

1ec5 commented Mar 21, 2018 •

edited

Loading