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

changed the default below layer for drawing route lines and manuever … #2878

Closed
wants to merge 1 commit into from

Conversation

cafesilencio
Copy link
Contributor

Description

#2872 street names beneath route lines

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Goal

The street names should appear above the route lines and turning arrows.

Implementation

Looking at the 0.42.4 source code the layer ID below which the route line was being drawn was mapbox-location however I don't see that layer when I list all of the layers in the style. I'm going to instead use the layer "road-label" as the default so that the street names will appear above the route line.

Screenshots or Gifs

Screenshot_20200430-164528

Testing

Please describe the manual tests that you ran to verify your changes

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have updated the CHANGELOG including this PR

@cafesilencio cafesilencio requested a review from a team April 30, 2020 23:48
@cafesilencio cafesilencio linked an issue Apr 30, 2020 that may be closed by this pull request
@codecov-io
Copy link

Codecov Report

Merging #2878 into master will increase coverage by 0.01%.
The diff coverage is 62.85%.

@@             Coverage Diff              @@
##             master    #2878      +/-   ##
============================================
+ Coverage     35.43%   35.45%   +0.01%     
- Complexity     2103     2106       +3     
============================================
  Files           545      545              
  Lines         19554    19567      +13     
  Branches       1841     1844       +3     
============================================
+ Hits           6929     6937       +8     
- Misses        11799    11802       +3     
- Partials        826      828       +2     

@@ -1,6 +1,7 @@
package com.mapbox.navigation.ui.route;

class RouteConstants {
static final String DEFAULT_ROUTE_LINE_LAYER_BELOW_ID = "road-label";

Choose a reason for hiding this comment

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

The Nav SDK cannot default to this layer id because a custom Map style might not have it. This would mean, that the route is most-likely end up being drawn on top of everything, including the puck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What layer should be used?

Choose a reason for hiding this comment

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

The ideal fallback flow should be:
provided layer id -> LocationComponentConstants.SHADOW_LAYER (if the layer exists) -> LocationComponentConstants.FOREGROUND_LAYER (if the layer exists) -> null

@cafesilencio cafesilencio marked this pull request as draft May 1, 2020 15:46
@cafesilencio
Copy link
Contributor Author

cafesilencio commented May 1, 2020 via email

@LukasPaczos
Copy link

So to address
the issue that was reported we should determine if our example apps should
be using passing a different layer in as a parameter.

That's right. Each implementation/example should provide the correct layer ID underneath which the route layer should be placed. This is heavily dependent on the used style, so we cannot effectively guess. The only convenience we can add is ensuring that, as a fallback, the route is at least placed below the location puck.

@cafesilencio
Copy link
Contributor Author

Based on feedback the solution implemented in this PR is not the right solution to pursue so I"m closing this.

@cafesilencio cafesilencio deleted the sb-2872-route-line-layer-elevation branch May 4, 2020 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Street names underneath route line
3 participants