Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

icon-padding supports array #1289

Merged
merged 8 commits into from
Jul 14, 2022
Merged

icon-padding supports array #1289

merged 8 commits into from
Jul 14, 2022

Conversation

drwestco
Copy link
Contributor

@drwestco drwestco commented Jun 1, 2022

Add new 'padding' type for style properties. Extend the behavior of icon-padding to support up to 4 values, including negative values, similar to how the CSS margin property works.

Replacement for #1237 - extending behavior of existing icon-padding instead of introducing a new property.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Manually test the debug page.
  • Suggest a changelog category: bug/feature/docs/etc. or "skip changelog".
  • Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

@drwestco drwestco mentioned this pull request Jun 1, 2022
Closed
9 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2022 •
edited
Loading

Bundle size report:

Size Change: +459 B
Total Size Before: 202 kB
Total Size After: 203 kB

Output file Before After Change
maplibre-gl.js 193 kB 193 kB +459 B
maplibre-gl.css 9.65 kB 9.65 kB 0 B
ℹ️ View Details
Source file Before After Change
src/style-spec/util/padding.ts 0 B 257 B +257 B
src/style-spec/validate/validate_padding.ts 0 B 242 B +242 B
src/style/style_layer/symbol_style_layer.ts 1.02 kB 1.15 kB +129 B
src/symbol/symbol_layout.ts 3.63 kB 3.76 kB +125 B
src/source/rtl_text_plugin.ts 798 B 909 B +111 B
node_modules/gl-matrix/esm/mat4.js 2.69 kB 2.77 kB +76 B
src/style-spec/util/interpolate.ts 172 B 239 B +67 B
src/util/performance.ts 699 B 750 B +51 B
src/style-spec/expression/index.ts 1.61 kB 1.66 kB +50 B
src/style-spec/function/index.ts 1.18 kB 1.23 kB +47 B
src/style-spec/util/color_spaces.ts 763 B 805 B +42 B
src/style-spec/expression/definitions/coercion.ts 702 B 736 B +34 B
src/util/actor.ts 813 B 846 B +33 B
src/style-spec/validate/validate.ts 393 B 417 B +24 B
src/style-spec/expression/parsing_context.ts 1.02 kB 1.03 kB +16 B
src/util/ajax.ts 2.38 kB 2.39 kB +15 B
node_modules/murmurhash-js/murmurhash2_gc.js 251 B 266 B +15 B
src/data/bucket/fill_attributes.ts 98 B 112 B +14 B
src/util/util.ts 1.86 kB 1.87 kB +12 B
src/style-spec/expression/values.ts 497 B 507 B +10 B
node_modules/gl-matrix/esm/vec2.js 246 B 256 B +10 B
src/style/style_layer/fill_extrusion_style_layer_properties.g.ts 181 B 191 B +10 B
src/style-spec/error/validation_error.ts 3.56 kB 3.57 kB +9 B
src/symbol/collision_feature.ts 377 B 386 B +9 B
node_modules/csscolorparser/csscolorparser.js 2.05 kB 2.06 kB +8 B
src/util/is_char_in_unicode_block.ts 876 B 883 B +7 B
src/util/script_detection.ts 1.65 kB 1.65 kB +7 B
src/style/style_layer/hillshade_style_layer_properties.g.ts 156 B 163 B +7 B
src/symbol/symbol_size.ts 637 B 644 B +7 B
src/render/image_atlas.ts 830 B 836 B +6 B
src/style-spec/expression/stops.ts 187 B 192 B +5 B
src/style-spec/validate/validate_object.ts 387 B 392 B +5 B
src/style-spec/validate/validate_color.ts 138 B 143 B +5 B
src/data/segment.ts 448 B 453 B +5 B
node_modules/@mapbox/vector-tile/lib/vectortilefeature.js 1.01 kB 1.01 kB +5 B
node_modules/@mapbox/vector-tile/lib/vectortile.js 181 B 186 B +5 B
src/style/style_layer/line_style_layer_properties.g.ts 274 B 279 B +5 B
src/style/style_layer/custom_style_layer.ts 486 B 491 B +5 B
src/style-spec/expression/types.ts 508 B 512 B +4 B
src/style-spec/expression/definitions/let.ts 434 B 438 B +4 B
src/style-spec/validate/validate_function.ts 1.14 kB 1.15 kB +4 B
src/style-spec/validate/validate_layout_property.ts 53 B 57 B +4 B
src/data/bucket/heatmap_bucket.ts 84 B 88 B +4 B
src/util/color_ramp.ts 318 B 322 B +4 B
src/geo/lng_lat.ts 583 B 587 B +4 B
src/style-spec/validate/validate_array.ts 355 B 358 B +3 B
src/style-spec/validate/validate_source.ts 615 B 618 B +3 B
src/style-spec/validate/validate_glyphs_url.ts 169 B 172 B +3 B
src/style/validate_style.ts 152 B 155 B +3 B
src/symbol/anchor.ts 167 B 170 B +3 B
node_modules/@mapbox/unitbezier/index.js 424 B 426 B +2 B
node_modules/@mapbox/point-geometry/index.js 628 B 630 B +2 B
src/style-spec/expression/types/formatted.ts 263 B 265 B +2 B
src/style-spec/expression/definitions/assertion.ts 584 B 586 B +2 B
src/style-spec/expression/definitions/collator.ts 386 B 388 B +2 B
src/style-spec/expression/definitions/match.ts 750 B 752 B +2 B
src/style-spec/validate/validate_filter.ts 620 B 622 B +2 B
src/style-spec/validate/validate_paint_property.ts 54 B 56 B +2 B
src/style-spec/validate/validate_formatted.ts 61 B 63 B +2 B
src/util/transferable_grid_index.ts 1.01 kB 1.02 kB +2 B
src/data/program_configuration.ts 2.61 kB 2.62 kB +2 B
src/style/style_layer/circle_style_layer_properties.g.ts 227 B 229 B +2 B
src/style/style_layer/heatmap_style_layer_properties.g.ts 135 B 137 B +2 B
src/util/classify_rings.ts 244 B 246 B +2 B
src/data/bucket/line_attributes.ts 119 B 121 B +2 B
node_modules/potpack/index.mjs 353 B 355 B +2 B
src/style/style_layer/symbol_style_layer_properties.g.ts 651 B 653 B +2 B
src/style/style_layer/heatmap_style_layer.ts 352 B 354 B +2 B
src/util/dictionary_coder.ts 151 B 153 B +2 B
src/util/browser.ts 451 B 452 B +1 B
src/style-spec/expression/parsing_error.ts 92 B 93 B +1 B
src/style-spec/expression/compound_expression.ts 732 B 733 B +1 B
src/style-spec/expression/definitions/step.ts 634 B 635 B +1 B
src/style-spec/expression/definitions/at.ts 377 B 378 B +1 B
src/style-spec/expression/definitions/number_format.ts 526 B 527 B +1 B
src/style-spec/expression/definitions/index.ts 1.62 kB 1.62 kB +1 B
src/style-spec/validate/validate_light.ts 288 B 289 B +1 B
src/style-spec/validate/validate_image.ts 61 B 62 B +1 B
src/data/extent.ts 31 B 32 B +1 B
node_modules/gl-matrix/esm/mat3.js 237 B 238 B +1 B
src/data/bucket/fill_bucket.ts 1.1 kB 1.1 kB +1 B
src/symbol/clip_line.ts 301 B 302 B +1 B
src/style/style_layer/raster_style_layer_properties.g.ts 172 B 173 B +1 B
src/style-spec/util/color.ts 318 B 317 B -1 B
src/style-spec/expression/definitions/literal.ts 313 B 312 B -1 B
src/style-spec/expression/definitions/var.ts 321 B 320 B -1 B
src/style-spec/expression/definitions/coalesce.ts 422 B 421 B -1 B
src/style-spec/expression/definitions/case.ts 437 B 436 B -1 B
src/style-spec/expression/definitions/image.ts 276 B 275 B -1 B
src/style-spec/expression/definitions/length.ts 332 B 331 B -1 B
src/data/array_types.g.ts 2.82 kB 2.82 kB -1 B
node_modules/murmurhash-js/index.js 67 B 66 B -1 B
src/data/feature_position_map.ts 561 B 560 B -1 B
src/data/load_geometry.ts 255 B 254 B -1 B
src/data/evaluation_feature.ts 96 B 95 B -1 B
src/data/bucket/circle_bucket.ts 970 B 969 B -1 B
node_modules/@mapbox/vector-tile/lib/vectortilelayer.js 434 B 433 B -1 B
src/data/bucket/symbol_attributes.ts 766 B 765 B -1 B
src/symbol/transform_text.ts 203 B 202 B -1 B
src/util/verticalize_punctuation.ts 587 B 586 B -1 B
src/symbol/one_em.ts 30 B 29 B -1 B
src/symbol/get_anchors.ts 571 B 570 B -1 B
src/style/style_layer/hillshade_style_layer.ts 141 B 140 B -1 B
src/style/style_layer/raster_style_layer.ts 67 B 66 B -1 B
src/source/tile_id.ts 1.14 kB 1.14 kB -1 B
src/util/config.ts 87 B 85 B -2 B
src/style-spec/validate/validate_enum.ts 209 B 207 B -2 B
src/style-spec/validate/validate_layer.ts 865 B 863 B -2 B
src/style-spec/validate_style.min.ts 293 B 291 B -2 B
src/util/web_worker_transfer.ts 952 B 950 B -2 B
src/style/evaluation_parameters.ts 388 B 386 B -2 B
src/data/bucket/fill_extrusion_bucket.ts 1.43 kB 1.43 kB -2 B
src/style/style_layer/line_style_layer.ts 821 B 819 B -2 B
node_modules/pbf/index.js 2.82 kB 2.82 kB -2 B
src/symbol/check_max_angle.ts 288 B 286 B -2 B
node_modules/tinyqueue/index.js 362 B 360 B -2 B
src/util/throttled_invoker.ts 212 B 210 B -2 B
src/geo/lng_lat_bounds.ts 616 B 614 B -2 B
node_modules/@mapbox/whoots-js/index.mjs 279 B 277 B -2 B
src/style-spec/expression/types/resolved_image.ts 135 B 132 B -3 B
src/style-spec/expression/definitions/within.ts 1.37 kB 1.37 kB -3 B
src/style-spec/expression/definitions/slice.ts 444 B 441 B -3 B
src/style-spec/expression/definitions/comparison.ts 841 B 838 B -3 B
src/style-spec/validate/validate_property.ts 549 B 546 B -3 B
src/style/zoom_history.ts 181 B 178 B -3 B
node_modules/gl-matrix/esm/common.js 182 B 179 B -3 B
node_modules/gl-matrix/esm/quat.js 156 B 153 B -3 B
src/data/bucket/pattern_bucket_features.ts 326 B 323 B -3 B
src/style/style_layer/fill_style_layer_properties.g.ts 195 B 192 B -3 B
src/data/bucket/line_bucket.ts 2.42 kB 2.41 kB -3 B
src/style/format_section_override.ts 308 B 305 B -3 B
src/util/resolve_tokens.ts 97 B 94 B -3 B
src/data/dem_data.ts 836 B 833 B -3 B
src/util/vectortile_to_geojson.ts 252 B 249 B -3 B
src/style-spec/expression/runtime_error.ts 113 B 109 B -4 B
src/style-spec/expression/definitions/format.ts 651 B 647 B -4 B
src/style-spec/validate/validate_expression.ts 456 B 452 B -4 B
src/style-spec/feature_filter/index.ts 893 B 889 B -4 B
src/style/style_layer/background_style_layer.ts 65 B 61 B -4 B
src/data/feature_index.ts 1.75 kB 1.75 kB -4 B
src/style-spec/expression/evaluation_context.ts 315 B 310 B -5 B
src/style/query_utils.ts 487 B 482 B -5 B
node_modules/ieee754/index.js 566 B 561 B -5 B
src/symbol/shaping.ts 3.63 kB 3.62 kB -5 B
src/style/properties.ts 1.88 kB 1.87 kB -6 B
src/shaders/encode_attribute.ts 86 B 80 B -6 B
src/util/image.ts 682 B 676 B -6 B
node_modules/@mapbox/vector-tile/index.js 95 B 89 B -6 B
src/symbol/quads.ts 1.89 kB 1.89 kB -6 B
src/style/style_layer/background_style_layer_properties.g.ts 116 B 110 B -6 B
src/style/style_layer/fill_extrusion_style_layer.ts 928 B 921 B -7 B
src/data/bucket/fill_extrusion_attributes.ts 134 B 125 B -9 B
node_modules/murmurhash-js/murmurhash3_gc.js 368 B 350 B -18 B
node_modules/earcut/src/earcut.js 2.81 kB 2.79 kB -18 B
src/util/tile_request_cache.ts 915 B 892 B -23 B
src/render/uniform_binding.ts 644 B 619 B -25 B
src/style/parse_glyph_pbf.ts 395 B 367 B -28 B
src/style-spec/expression/definitions/interpolate.ts 1.26 kB 1.22 kB -40 B
src/style/create_style_layer.ts 384 B 339 B -45 B
node_modules/gl-matrix/esm/vec4.js 519 B 419 B -100 B
node_modules/gl-matrix/esm/vec3.js 975 B 855 B -120 B

@HarelM
Copy link
Collaborator

HarelM commented Jun 2, 2022

This looks very good!
Can you share why a "padding" type is needed?
Is it because we can't write in the style "number or number array"?

@drwestco
Copy link
Contributor Author

drwestco commented Jun 2, 2022 •
edited
Loading

tl;dr - sticking with existing array, number types quickly complicated the logic.

I started by just using "type": "array", "value": "number". To support using just a number, I added a new attribute "allow-bare-value": true, then added max-length to go with existing min-length and length attributes. That set of attributes is enough to describe the number | Array<number> aspect, though it's fairly complicated to validate and parse styles using any combination of those attributes.

It was more complicated once I started looking into expressions and especially interpolation. Ideally, you'd want to be able to interpolate between padding: [2] and padding: [2, 2, 4, 2] to adjust just the bottom padding. Existing array interpolation requires each stop to have the same number of elements. For something like padding/margin, [2] is just shorthand for [2, 2, 2, 2] - that expansion is specific to padding, and doesn't make sense for array properties in general. At that point, switching to a Padding type made more sense, following the pattern used by Color properties written as strings.

I can see potential for a future addition of icon-margin in addition to icon-padding. Padding+margin would be used for collisions, the pair together determining how close two labels can appear. Padding alone would determine the hit test region for a single label. In that case, both style properties can share the Padding type. Not sure if there's a better name to use for that type - more generic term for [top, right, bottom, left]?

@HarelM
Copy link
Collaborator

HarelM commented Jun 3, 2022

I see, thanks for the extra info!
Few names that comes to mind for the "new" type:
Container, Rect, Wrapper, Border, BorderRect.
Not sure they are better than padding though.
I'll take a deeper look into the code now with the info you provided.

HarelM
HarelM requested changes Jun 8, 2022
View reviewed changes
Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

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

See my comments.
Overall looks very good!!
I think more tests are needed in the files you added the padding definition, parsing, serialization etc...

HarelM
HarelM reviewed Jun 21, 2022
View reviewed changes
debug/setstyle.html Outdated Show resolved Hide resolved
drwestco added 3 commits June 21, 2022 14:46
@drwestco
icon-padding supports array
6fa91c5 
Add new 'padding' type for style properties. Extend the behavior of icon-padding to support up to 4 values, including negative values, similar to how the CSS margin property works.
@drwestco
Add padding expression tests
9092908
@drwestco
No need to serialize padding
1ce5756
HarelM
HarelM approved these changes Jun 23, 2022
View reviewed changes
Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

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

Thanks for all the effort!

@HarelM
Copy link
Collaborator

HarelM commented Jul 13, 2022

@drwestco Can you please resolve conflicts? I think this is ready to be merged right? We removed all the serialization code in another PR, right?

@drwestco
Copy link
Contributor Author

Yes. The serialization code was removed in PR #1315

HarelM
HarelM reviewed Jul 13, 2022
View reviewed changes
src/style/style_layer/symbol_style_layer.ts
export type SymbolPadding = [number, number, number, number];

export function getIconPadding(layout: PossiblyEvaluated<SymbolLayoutProps, SymbolLayoutPropsPossiblyEvaluated>, feature: SymbolFeature, canonical: CanonicalTileID, pixelRatio = 1): SymbolPadding {
// Support text-padding in addition to icon-padding? Unclear how to apply asymmetric text-padding to the radius for collision circles.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I know what to do with this comment - is this a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Not sure there's a use case for extending text-padding like I did icon-padding. I can remove the comment and open an issue if that makes more sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, I think an issue would be better...

@HarelM
Copy link
Collaborator

HarelM commented Jul 13, 2022

Last few nit picking I stumbled upon before this can be merged. Sorry for all the back-and-forth...

@HarelM HarelM merged commit 7600fd0 into maplibre:main Jul 14, 2022
@HarelM
Copy link
Collaborator

HarelM commented Jul 14, 2022

Thanks a lot for bearing with all my annoying comments! :-)

@drwestco drwestco deleted the stylePaddingType branch July 14, 2022 16:48
@drwestco drwestco mentioned this pull request Jul 14, 2022
Open
@HarelM HarelM mentioned this pull request Nov 2, 2022
Merged
@HarelM HarelM mentioned this pull request May 7, 2024
Merged
@random3940 random3940 mentioned this pull request Sep 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbu42 xabbu42 xabbu42 left review comments

@HarelM HarelM HarelM approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drwestco @HarelM @xabbu42