[Maps] Convert maki icons to SDF sprites on-the-fly #119245

nickpeihl · 2021-11-19T22:03:05Z

Summary

Enables dynamic styling of Maki icons in Maps by converting the SVG Maki icons to PNG sprites using signed distance functions.

The SVG icon is drawn on an HTML Canvas and a signed distance function algorithm is applied to the ImageData from the Canvas. The resulting imageData is added to the MapLibre Map as an SDF sprite.

Previously the maki icons were converted into a spritesheet in the @elastic/maki module using the @elastic/spritezero library. Moving the SDF sprite generation to the browser simplifies the process for adding new icons as well as laying the groundwork for users to use their own SVG icons in the future.

For comparison, the screenshot on the left is from this PR. The screenshot on the right shows the same map from Kibana 7.15.0.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-11-19T22:03:07Z

Pinging @elastic/kibana-gis (Team:Geo)

nickpeihl · 2021-11-19T22:06:22Z

x-pack/plugins/maps/public/connected_components/mb_map/mb_map.tsx

 import { syncLayerOrder } from './sort_layers';

 import {
-  addSpriteSheetToMapFromImageData,
-  loadSpriteSheetImageData,
  removeOrphanedSourcesAndLayers,
 } from './utils';


With this change, addSpriteSheetToMapFromImageData and loadSpriteSheetImageData are now only used by the VectorTileLayer module. Not sure if we want to continue to export these functions from utils or if we should move them to vector_tile_layer.tsx?

I would still keep then in a separate file, How about just moving them into vector_tile_layer folder?

nreese

Thanks for moving sdf loading into Kibana. This is great that custom icons and maki icons will have a more similar code path.

nreese · 2021-11-22T15:18:46Z

x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.js

 export function getMakiSymbolSvg(symbolId) {
  if (!SYMBOLS[symbolId]) {
    throw new Error(`Unable to find symbol: ${symbolId}`);
  }
  return SYMBOLS[symbolId];
 }

+export async function loadMakiIconInMap(symbolId, mbMap) {
+  const pixelRatio = Math.floor(window.devicePixelRatio);


How about moving pixelRatio to within the scope of if statement. syncIconWithMb, where loadMakiIconInMap is called, gets called any time layerList state changes (which is a lot). So avoiding any memory creation when the image already exists will help reduce GC.

nreese · 2021-11-22T15:27:34Z

x-pack/plugins/maps/public/classes/styles/vector/properties/dynamic_icon_property.tsx

+        ...(iconPaletteId ? getIconPalette(iconPaletteId) : []),
+        ...(customIconStops ? customIconStops.map(({ icon }) => icon) : []),
+      ];
+      Promise.all(


Can you add logic to handle exceptions. For example, loadMakiIconInMap could throw an error if getMakiSymbolSvg gets an unknown id. I think the best way to handle the error is to surface a warning toast.

nreese · 2021-11-22T15:27:42Z

x-pack/plugins/maps/public/classes/styles/vector/properties/static_icon_property.ts

 import { IconStaticOptions } from '../../../../../common/descriptor_types';

 export class StaticIconProperty extends StaticStyleProperty<IconStaticOptions> {
  syncIconWithMb(symbolLayerId: string, mbMap: MbMap, iconPixelSize: number) {
    const symbolId = this._options.value;
-    mbMap.setLayoutProperty(symbolLayerId, 'icon-anchor', getMakiSymbolAnchor(symbolId));
-    mbMap.setLayoutProperty(symbolLayerId, 'icon-image', getMakiIconId(symbolId, iconPixelSize));
+    loadMakiIconInMap(symbolId, mbMap).then(() => {


Can you add logic to handle exceptions. For example, loadMakiIconInMap could throw an error if getMakiSymbolSvg gets an unknown id. I think the best way to handle the error is to surface a warning toast.

thomasneirynck · 2021-11-29T21:15:13Z

x-pack/plugins/maps/public/classes/styles/vector/properties/dynamic_icon_property.tsx

+        ...(iconPaletteId ? getIconPalette(iconPaletteId) : []),
+        ...(customIconStops ? customIconStops.map(({ icon }) => icon) : []),
+      ];
+      Promise.all(


Discussed offline with @nickpeihl @nreese .

Would move the registration of the icons to the bootstrap phase when the maps get initialized. Basically, similar to how it used to be.

+1 on keeping syncIconWithMb synchronous and not introduce any callbacks or other async-code in here.

nickpeihl · 2021-11-30T17:29:32Z

@elasticmachine merge upstream

kibanamachine · 2021-11-30T17:29:35Z

merge conflict between base and head

nickpeihl · 2021-12-02T19:40:58Z

x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.js

 import { parseXmlString } from '../../../../common/parse_xml_string';
 import { SymbolIcon } from './components/legend/symbol_icon';
 import { getIsDarkMode } from '../../../kibana_services';
+import { MAKI_ICONS } from './maki_icons';


Each icon in maki_icons can have multiple properties like svg and label with the possibility of adding more such as anchor to remove hard-coding icon ids in kibana like in the getMakiAnchor function.

nreese

I have discovered a bug where going from icon size 14 to 15 results in smaller icons. Notice the screen shots below and how the icons are bigger when size is 14. The reason for this problem is that the icons used to have two sizes, 11px and 15px and that the swapping between the two would create a smooth transition. To fix the problem, getIconPixelSize needs to be removed from StaticSizeProperty and DynamicSizeProperty. Then in syncIconSizeWithMb, halfIconPixels should just be hard coded to 7.5 since all icons are 15 pixels in height.

nreese · 2021-12-03T15:35:31Z

x-pack/plugins/maps/public/connected_components/mb_map/mb_map.tsx

 import { syncLayerOrder } from './sort_layers';

-import {
-  addSpriteSheetToMapFromImageData,


Should addSpriteSheetToMapFromImageData and loadSpriteSheetImageData functions be moved from ./utils to classes/layers/vector_tile_layer since that is the only place they are used?

nickpeihl · 2021-12-03T19:14:00Z

NOTICE.txt

@@ -295,7 +295,7 @@ MIT License http://www.opensource.org/licenses/mit-license
 ---
 This product includes code that is adapted from mapbox-gl-js, which is
 available under a "BSD-3-Clause" license.
-https://github.com/mapbox/mapbox-gl-js/blob/master/src/util/image.js
+https://github.com/mapbox/mapbox-gl-js/blob/v1.13.2/src/util/image.js


Updated with link to open source code. The code in the master branch is now proprietary due to a licensing change in v2.0.

nickpeihl · 2021-12-03T19:15:09Z

x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.js

-const LARGE_MAKI_ICON_SIZE_AS_STRING = LARGE_MAKI_ICON_SIZE.toString();
-export const SMALL_MAKI_ICON_SIZE = 11;
-export const HALF_LARGE_MAKI_ICON_SIZE = Math.ceil(LARGE_MAKI_ICON_SIZE);
+const MAKI_ICON_SIZE = 16;


An even number renders better so the icon size is 16 not 15.

nreese

LGTM, thanks for making these changes and pathing the way for custom icons. I think the slight loss in icon crispness is worth it for custom icons
code review, tested in chrome

nickpeihl · 2021-12-07T16:52:45Z

@elasticmachine merge upstream

kibanamachine · 2021-12-07T16:52:47Z

merge conflict between base and head

kibana-ci · 2021-12-07T22:22:17Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`maps`	801	807	+6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`maps`	2.6MB	2.6MB	-69.0KB

Unknown metric groups

miscellaneous assets size

id	before	after	diff
`maps`	958.9KB	530.1KB	-428.9KB

History

💔 Build #11919 failed 498f873
💚 Build #11077 succeeded 2d95d38
💔 Build #10760 failed 805a9e3
💔 Build #10167 failed f91a43a
💔 Build #10069 failed 0e6edf9
💔 Build #9871 failed 8372c05

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Convert SVG maki icons to SDF on-the-fly

Convert SVG maki icons to SDF on-the-fly

585137c

nickpeihl added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.1.0 labels Nov 19, 2021

nickpeihl requested a review from a team as a code owner November 19, 2021 22:03

nickpeihl commented Nov 19, 2021

View reviewed changes

Lint fixes

d3e82b8

nreese reviewed Nov 22, 2021

View reviewed changes

Lint fix again

574708f

thomasneirynck self-requested a review November 29, 2021 18:53

thomasneirynck reviewed Nov 29, 2021

View reviewed changes

nickpeihl added 2 commits November 29, 2021 16:21

Load SDF maki icons during map bootstrap

8372c05

Maki sprites no longer have pixel sizes when added to mbMap

0e6edf9

nickpeihl added 3 commits November 30, 2021 12:05

Merge branch 'main' of https://github.com/elastic/kibana into sdf-maki

f91a43a

Remove @elastic/maki dependency

b652b35

Merge branch 'main' of https://github.com/elastic/kibana into sdf-maki

805a9e3

nickpeihl commented Dec 2, 2021

View reviewed changes

nickpeihl requested review from nreese and thomasneirynck December 2, 2021 19:41

nreese reviewed Dec 3, 2021

View reviewed changes

nickpeihl added 2 commits December 3, 2021 10:57

Review feedback

9bf40cc

Merge branch 'main' of https://github.com/elastic/kibana into sdf-maki

2d95d38

nickpeihl commented Dec 3, 2021

View reviewed changes

nreese approved these changes Dec 6, 2021

View reviewed changes

nickpeihl added 2 commits December 7, 2021 12:05

Merge branch 'main' of https://github.com/elastic/kibana into sdf-maki

498f873

Fix package.json

e3ce865

nickpeihl merged commit c838709 into elastic:main Dec 7, 2021

kibanamachine added the backport:skip This commit does not require backporting label Dec 7, 2021

TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021

[Maps] Convert maki icons to SDF sprites on-the-fly (elastic#119245)

671055f

* Convert SVG maki icons to SDF on-the-fly

gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022

[Maps] Convert maki icons to SDF sprites on-the-fly (elastic#119245)

1804a61

* Convert SVG maki icons to SDF on-the-fly

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Maps] Convert maki icons to SDF sprites on-the-fly #119245

[Maps] Convert maki icons to SDF sprites on-the-fly #119245

nickpeihl commented Nov 19, 2021

elasticmachine commented Nov 19, 2021

nickpeihl Nov 19, 2021

nreese Nov 22, 2021

nreese left a comment

nreese Nov 22, 2021

nreese Nov 22, 2021

nreese Nov 22, 2021

thomasneirynck Nov 29, 2021

nickpeihl commented Nov 30, 2021

kibanamachine commented Nov 30, 2021

nickpeihl Dec 2, 2021

nreese left a comment •

edited

Loading

nreese Dec 3, 2021

nickpeihl Dec 3, 2021

nickpeihl Dec 3, 2021

nreese left a comment

nickpeihl commented Dec 7, 2021

kibanamachine commented Dec 7, 2021

kibana-ci commented Dec 7, 2021

miscellaneous assets size

[Maps] Convert maki icons to SDF sprites on-the-fly #119245

[Maps] Convert maki icons to SDF sprites on-the-fly #119245

Conversation

nickpeihl commented Nov 19, 2021

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Nov 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese left a 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

Choose a reason for hiding this comment

nickpeihl commented Nov 30, 2021

kibanamachine commented Nov 30, 2021

Choose a reason for hiding this comment

nreese left a comment • 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

nreese left a comment

Choose a reason for hiding this comment

nickpeihl commented Dec 7, 2021

kibanamachine commented Dec 7, 2021

kibana-ci commented Dec 7, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

miscellaneous assets size

History

nreese left a comment •

edited

Loading