-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
8971073
to
41ea76b
Compare
20b4355
to
8afa0ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just have a couple notes on the implementation choices:
-
The use of
std::map
andstd::unrodered_map
is likely to provide unsatisfactory performance when there are large changes in feature state - for example: https://bl.ocks.org/asheemmamoowala/014fa054005f3a46a10fd6fc306f551f -
The use of
Convertible
as the input parameter forsetFeatureState
makes it very broad, and hard to force type checking on the input data. The platform APIs will probably want to expose this functionality using platform-appropriate types (for exampleNSMutableDictionary
on osx) which would create an extra step to make those intoConvertible
objects.
cc @julianrex @tobrun for input on the interface for platform SDKs.
96d1864
to
1d64fe3
Compare
Adding a screenshot of feature state demo on mbgl-glfw app. The demo implements the same hover effect as this GL-JS example. |
390781d
to
185e70c
Compare
cbcf276
to
388673a
Compare
388673a
to
63ab724
Compare
void getFeatureState(FeatureState& state, | ||
const std::string& sourceID, | ||
const optional<std::string>& sourceLayerID, | ||
void setFeatureState(const std::string& sourceID, const optional<std::string>& sourceLayerID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I prefer old style (chromium) where each argument is on a new line. @tmpsantos could we add exception for this to clang-format config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmpsantos could we add these to clang format?
BinPackParameters: false
AllowAllArgumentsOnNextLine: false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've agreed with @jmalanen that he will investigate moving feature state into GeometryTileFeature
Fixes: #11846