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

Stretchable icons #16030

Merged
merged 13 commits into from
Jan 15, 2020
Merged

Stretchable icons #16030

merged 13 commits into from
Jan 15, 2020

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Dec 9, 2019

Fixes #16017

Implements stretchable icons, porting from JS.

@kkaefer kkaefer added feature GL JS parity For feature parity with Mapbox GL JS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold Core The cross-platform C++ core, aka mbgl labels Dec 9, 2019
@kkaefer
Copy link
Contributor Author

kkaefer commented Dec 10, 2019

Remaining questions:

  • We're using std::vector for storing stretches, which might be using quite a bit of memory. Ideally, we'd use a "small vector" that stores 0-1 elements inline to avoid heap allocations.
  • We're storing texture coordinates as integers in Native. I believe we do that as well in JS, which means that we'll have to remove the option to pass floating point numbers in stretches.

@kkaefer kkaefer marked this pull request as ready for review December 10, 2019 17:03
@kkaefer kkaefer removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Dec 10, 2019
@kkaefer kkaefer force-pushed the stretchable-icons branch 2 times, most recently from 2622cb0 to 23a78d7 Compare December 11, 2019 11:45
@ansis
Copy link
Contributor

ansis commented Dec 11, 2019

We're storing texture coordinates as integers in Native. I believe we do that as well in JS, which means that we'll have to remove the option to pass floating point numbers in stretches.

Yep, should we do something to enable some fractional values? I could see half pixel values being useful but not sure about anything beyond that.

Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

looks great! few nits

include/mbgl/util/exception.hpp Show resolved Hide resolved
platform/android/src/map/image.cpp Show resolved Hide resolved
platform/android/src/native_map_view.cpp Show resolved Hide resolved
src/mbgl/text/quads.cpp Outdated Show resolved Hide resolved
@ansis
Copy link
Contributor

ansis commented Dec 11, 2019

I took a look a look at the core logic part and I checked that all the followup fixes I made in -js are also included here. Those look good (assuming the TODOs are done). I'll leave the rest of the review to Alex

@chloekraw chloekraw added this to the release-unicorn milestone Dec 18, 2019
@alexshalamov alexshalamov force-pushed the stretchable-icons branch 2 times, most recently from f9a7817 to d7b4073 Compare January 3, 2020 15:18
@kkaefer
Copy link
Contributor Author

kkaefer commented Jan 6, 2020

@alexshalamov thanks for rebaselining!

We're using a lot of std::vectors that have no or only one element. my fear is that std::vector is using too much memory + heap allocations. Is there a tutorial for how I could benchmark this in GL native? I'm looking at https://github.com/abseil/abseil-cpp/blob/master/absl/container/inlined_vector.h; have we considered including abseil (or some other library) that would give us these types?

@alexshalamov alexshalamov added the needs changelog Indicates PR needs a changelog entry prior to merging. label Jan 13, 2020
@alexshalamov alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Jan 14, 2020
@alexshalamov
Copy link
Contributor

@kkaefer I created an issue #16118 to follow-up idea of using inlined vectors.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl feature GL JS parity For feature parity with Mapbox GL JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

port stretchable icons from -js
4 participants