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.

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

Dynamic anchors #893

Merged
merged 55 commits into from
Aug 16, 2016
Merged

Dynamic anchors #893

merged 55 commits into from
Aug 16, 2016

Conversation

hjanetzek
Copy link
Member

@hjanetzek hjanetzek commented Aug 2, 2016

Continuing #842

Since the changes of tile-label-placement branch introduced incremental collision testing it's possible to try fallback anchors directly instead of going through the fallbacks during multiple frames.

TODO

  • When the parent label has multiple anchors the child label must be updated() after the parent has been placed
  • Bug in alignFromParent where the parent's offset is constantly added to childs m_offset
  • Should offset for child labels be ignored since it conflicts the offset from the parent?

BUGS

  • icon:text:transition [show,hide] time: form is not parsed correctly (old bug)

@hjanetzek hjanetzek force-pushed the dynamic-anchors branch 3 times, most recently from c5ea68b to 39e0c2a Compare August 3, 2016 12:56
@hjanetzek hjanetzek changed the title [rfc] Dynamic anchors Dynamic anchors Aug 3, 2016
@hjanetzek hjanetzek force-pushed the dynamic-anchors branch 7 times, most recently from aa66108 to c2c507b Compare August 8, 2016 18:14
@hjanetzek hjanetzek mentioned this pull request Aug 8, 2016
7 tasks
- don't apply anchor offset in updateScreenTransform so that
label.transform.state.screenPos can be reused while switching
anchors

- make sprite-quads relative to center (to be consistent with
  text labels)
@@ -278,7 +278,7 @@ void PointStyleBuilder::addFeature(const Feature& _feat, const DrawRule& _rule)

size_t textStart = textLabels.size();

textStyleBuilder.addFeatureCommon(_feat, _rule, true);
if (!textStyleBuilder.addFeatureCommon(_feat, _rule, true)) { return; }
Copy link
Member

Choose a reason for hiding this comment

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

👍

@tallytalwar
Copy link
Member

tallytalwar commented Aug 10, 2016

Updating fallback anchor on the same frame does improve the feeling of jumpiness/flickering labels we were seeing before. But its still noticeable. (try with bubble-wrap manhattan area). Might require some minor ironing here to get a crispy transitions. Otherwise code is fine.

Will play with it a bit more on bubble-wrap.

@@ -560,7 +560,7 @@ std::shared_ptr<Texture> SceneLoader::fetchTexture(const std::string& name, cons
auto ptr = (unsigned char*)(rawData.data());
size_t dataSize = rawData.size();
std::lock_guard<std::mutex> lock(m_textureMutex);
auto texture = scene->getTexture(name);
auto texture = scene->getTexture(name);
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, can you fix the indentation here? (also line# 807)

Copy link
Member Author

Choose a reason for hiding this comment

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

M-x whitespace-cleanup did replace tabs with spaces. @karimnaaji fix your editor! :p

Copy link
Member

Choose a reason for hiding this comment

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

Bubble Wrap v4.1.0 does use the proper anchor fallbacks now, but that
version is not yet used in Eraser Map the app (it’s pegged to v3.x).

On Wed, Aug 10, 2016 at 2:26 PM, Hannes Janetzek [email protected]
wrote:

In core/src/scene/sceneLoader.cpp
#893 (comment):

@@ -560,7 +560,7 @@ std::shared_ptr SceneLoader::fetchTexture(const std::string& name, cons
auto ptr = (unsigned char*)(rawData.data());
size_t dataSize = rawData.size();
std::lock_guardstd::mutex lock(m_textureMutex);

  •           auto texture = scene->getTexture(name);
    
  •                            auto texture = scene->getTexture(name);
    

M-x whitespace-cleanup did replace tabs with spaces. @karimnaaji
https://github.com/karimnaaji fix your editor! :p


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/tangrams/tangram-es/pull/893/files/562f172b3d217c7fcbdd754bb5d0102a0e681b0c#r74334094,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0EO7xuIFHvnlY_QmQnmWk2qgWBb8ovks5qekIKgaJpZM4Ja2Xw
.

Copy link
Member Author

@hjanetzek hjanetzek Aug 10, 2016

Choose a reason for hiding this comment

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

Nice! looking good - even though I'm more in favor of four point-placement for better icon-text association :)

@varun: of course there is now more jumping as anchor fallbacks are not coordinated between zoom-levels. definitely something to investigate in future

Oh, wrong thread..

@hjanetzek
Copy link
Member Author

Did you compare side-by-side eraser-map with master? Could be that you notice it now more while focusing on the labels. It should make no difference as eraser-map doesn't use anchor fallbacks, or does it now?

@hjanetzek
Copy link
Member Author

Last commit should improve labels flickering on zoom-level jumps when no transition effect is set

@hjanetzek hjanetzek force-pushed the dynamic-anchors branch 2 times, most recently from 752fdfe to 576a870 Compare August 10, 2016 22:19
@tallytalwar
Copy link
Member

I am fine with merging this now. Changing default to use 4 anchor points instead of 8 makes the transitioning much more smoother.

Going to merge this, if no one has any other issue?
@karimnaaji you would wanna look at some of the last changes here?

@karimnaaji
Copy link
Member

@tallytalwar hold on this while I'm looking at the last changes.

@karimnaaji
Copy link
Member

@hjanetzek are you aware of the issue exposed in this video? (look at "underground pizza" label). It seems that the fallback does not reset properly the fading transition.

Another thing I noticed; sometimes the fallback involves a fading transition, sometimes not, looking at this more closely.

@hjanetzek
Copy link
Member Author

@karimnaaji I think this effect came from the newly added fade_out->fade_in state change. Which was also missing to reset the fade effect.

- needs more thought, fade_in time must match the current alpha
@karimnaaji
Copy link
Member

All good to me here!

- fixes some child labels from not becoming visible on zoom-level switch
@hjanetzek hjanetzek merged commit d8d76eb into master Aug 16, 2016
@hjanetzek hjanetzek deleted the dynamic-anchors branch August 16, 2016 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants