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

Reflect changes in icon size immediately #6932

Merged
merged 1 commit into from
Nov 11, 2016
Merged

Conversation

ivovandongen
Copy link
Contributor

Fixes #6903

@ivovandongen ivovandongen added the Core The cross-platform C++ core, aka mbgl label Nov 6, 2016
@ivovandongen ivovandongen self-assigned this Nov 6, 2016
@mention-bot
Copy link

@ivovandongen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tobrun, @cammace and @zugaldia to be potential reviewers.

@ivovandongen
Copy link
Contributor Author

@jfirebaugh The problem seems to be that the style needs to be recalculated for the change to appear. Recalculating on all layout property changes is not very efficient, we can limit this to SymbolLayers or add another signal to the observer specifically for attributes that need style recalculation.

@ivovandongen
Copy link
Contributor Author

Added test case in: mapbox/mapbox-gl-test-suite#161

@ivovandongen ivovandongen mentioned this pull request Nov 7, 2016
@jfirebaugh
Copy link
Contributor

The regression test passes for me even without the changes here. Can you see what's needed to get that to fail, so we know it's a good regression test?

@ivovandongen
Copy link
Contributor Author

@jfirebaugh I've been trying to get the test to fail without success. Whereas the test activity I made clearly reproduces the error, the render tests just wont. Could this be a difference in render modes (still/continuous)?

@jfirebaugh
Copy link
Contributor

That's possible. It'll probably require C++ debugging to track down what's going on here. I'll try setting up the equivalent scenario in macosapp.

@jfirebaugh
Copy link
Contributor

@ivovandongen -- I figured out why the render test was passing. There were a variety of places in which the test harness called map.setZoom, triggering a recalculate that wouldn't normally happen. They are all unnecessary and could have obscured other issues, so I'm eliminating them in #6989, mapbox/mapbox-gl-test-suite#164, and #6990. After those PRs merge, the test case should fail as expected.

As to why recalculation is necessary, it comes down to this special logic for text-size / icon-size. Your fix does resolve the issue, but it would be good to find a fix that triggers recalculation only for these two properties. I'm not sure what the best way to do that is yet. Do you have any ideas?

@ivovandongen
Copy link
Contributor Author

@jfirebaugh Thanks for looking into this!

Your fix does resolve the issue, but it would be good to find a fix that triggers recalculation only for these two properties. I'm not sure what the best way to do that is yet. Do you have any ideas?

No clean ideas yet. I see two options:

  • Only trigger recalculation for SymbolLayers (in Style::onLayerLayoutPropertyChanged). Least impact, but not very efficient.
  • Add an optional parameter to LayerObserver::onLayerLayoutPropertyChanged() to signal recalculation is needed. The symbol layer would set this when icon or text size is changed. Not very nicely separated though.

I think I would prefer the latter. I'll update the pr accordingly.

@ivovandongen ivovandongen force-pushed the 6903-icon-size branch 2 times, most recently from 31837ff to c522b7b Compare November 10, 2016 15:58
@@ -169,7 +169,7 @@ void SymbolLayer::setIconSize(PropertyValue<float> value) {
if (value == getIconSize())
return;
impl->layout.iconSize.set(value);
impl->observer->onLayerLayoutPropertyChanged(*this);
impl->observer->onLayerLayoutPropertyChanged(*this, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

symbol_layer.cpp is a generated file, so this change will get overwritten.

Instead of a boolean, how about a const char * property, with name from style spec, e.g. "icon-size". We can update layer.cpp.ejs to add this parameter for every setter, and do exact matches in Style::onLayerLayoutPropertyChanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

symbol_layer.cpp is a generated file, so this change will get overwritten.

Of course.. I was a bit too hasty there.

Instead of a boolean, how about a const char * property, with name from style spec

Makes sense, I'll start on that now.

@ivovandongen
Copy link
Contributor Author

@jfirebaugh I've updated the PR according to #6932 (comment)

auto update = Update::Layout;

//Recalculate the style for certain properties
bool needsRecalculation = strcmp(property, "icon-size") || strcmp(property, "text-size");
Copy link
Contributor

Choose a reason for hiding this comment

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

strcmp returns 0 if they are equal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks again! Updated the PR and rebased on master.

@jfirebaugh
Copy link
Contributor

Last step is to merge mapbox/mapbox-gl-test-suite#161 and then update the SHA in package.json.

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 runtime styling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants