-
Notifications
You must be signed in to change notification settings - Fork 245
Hot fix for https://github.com/tangrams/tangram-es/issues/1167 #1185
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.
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
Conversation
core/src/labels/labels.cpp
Outdated
| m_needUpdate |= label->evalState(dt); | ||
| label->addVerticesToMesh(); | ||
| } | ||
| m_allLabels.emplace_back(label.get(), tile, isProxy); |
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.
I see the bug of non colliding labels are not checked when retrieving the labels for selection color. Instead of putting all visible labels for selection color checking, does it make sense to only put labels with "interactive" property set for selection color checking? Say if only pois are marked interactive, it will be wasteful to check for all text labels for selection colors.
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.
I think this can be identified by checking the featureId of the label to be non zero. If its non-zero it was marked interactive, as a valid selectionColor was set for this.
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.
Yep that's a good point! @hjanetzek also proposed it, I will clean up that branch since it's a lot of debug code and will update for that.
|
@blair1618 this is not an egregious hack, this is the actual fix for the problem explained here #1184 (comment). |
tallytalwar
left a comment
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.
Yoda: not hot-fix but a proper fix this is, I say.
hjanetzek
left a comment
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.
late to the party... everything is merged already
This resolves the issue as far as I can tell. The surrounding code is too complex for me to tell whether this is an egregious hack. If it isn't, I recommend merging this so that we can provide the Mapzen Android SDK with an updated release.