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

improved dashed lines #938

Closed
wants to merge 24 commits into from
Closed

improved dashed lines #938

wants to merge 24 commits into from

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Jan 16, 2015

A new attempt improving dashed lines. Replaces #649

no sliding

Dashes no longer slid along the lines as you zoom. Instead they stretch and then reset at every zoom.

scaled by width

Dashes size units are now scaled up by the line width. This should make it easier to use dashes that look right at every zoom without creating functions for them. So this is what "line-dasharray": [1, 1] looks like at line widths 2, 8, 16:

screen shot 2015-01-16 at 1 55 23 pm screen shot 2015-01-16 at 1 55 52 pm screen shot 2015-01-16 at 1 56 11 pm

If you still need functions, you can use them. They look like this:

"line-dash-array": {
    "stops": [ [0, [1.2, 0.8]], [15, [1.7, 0.3]]]
}

longer patterns

Previously dash arrays could only have on dash length value and one gap length. Dash patterns can now be any length. "line-dasharray": [5, 1, 1, 1, 1, 1, 1, 1]:

screen shot 2015-01-16 at 1 50 30 pm

round dashes

The ends of the dashes now obey the line-cap layout property, so you can have rounded dashes:

screen shot 2015-01-16 at 2 03 18 pm screen shot 2015-01-16 at 2 04 02 pm


Style spec changes: mapbox/mapbox-gl-style-spec#234
Native port: mapbox/mapbox-gl-native#761
Updated tests: mapbox/mapbox-gl-test-suite#12
Style migration: cutting-room-floor/mapbox-gl-style-lint#37
Styles: mapbox/mapbox-gl-styles#24

TODO

  • update render tests
  • style migration scripts, migrate styles. This is going to be tricky for dashes that used to be functions.

ansis added 6 commits January 13, 2015 17:16
Instead of spinning, dashes stretch before reseting at every round zoom.
Dash and space length units are now in terms of the line width, not pixels.
Dasharrays can be any length, not just 2.
Lines with "line-cap": "round" will have round dashes.
This might get re-added if we change how line patterns are implemented.
@nickidlugash
Copy link

@ansis Did we decide to scale dashes by line-gap-width? – I think it makes more sense for dashes to always be scaled by line-width (#860 (comment)).

@ansis
Copy link
Contributor Author

ansis commented Jan 16, 2015

@ansis Did we decide to scale dashes by line-gap-width? – I think it makes more sense for dashes to always be scaled by line-width (#860 (comment)).

It's implemented, but can easily be reverted. I don't have enough experience to have an opinion about this. @jfirebaugh are you fine with reverting a5fb9f9, or do you still think scaling by gap-width is better?

@ljbade
Copy link

ljbade commented Jan 17, 2015

Love the improvements, particularly the more flexible dash pattern.

@ansis
Copy link
Contributor Author

ansis commented Jan 21, 2015

Updated tests: mapbox/mapbox-gl-test-suite#12
Style migration: cutting-room-floor/mapbox-gl-style-lint#37
Styles: mapbox/mapbox-gl-styles#24

remaining:

  • choose whether dash size should be scaled by line-gap-width
  • review (if anyone wants to)

@jfirebaugh
Copy link
Contributor

If we always scale by line-width, is it still possible to manually scale the values to get exactly aligned casing and fill dashes? I'm worried that float rounding or something will screw that up.

@nickidlugash
Copy link

choose whether dash size should be scaled by line-gap-width

My vote is to revert this.

Can't wait to try out the new dashes!

@ansis
Copy link
Contributor Author

ansis commented Jan 21, 2015

If we always scale by line-width, is it still possible to manually scale the values to get exactly aligned casing and fill dashes? I'm worried that float rounding or something will screw that up.

Yeah, it seems to work fine.

Outer: "line-dasharray": [2.5, 1.5, 2.5], "line-width": 2
Inner: "line-dasharray": [4.5, 4, 4.5], "line-width": 1

screen shot 2015-01-20 at 7 38 28 pm

My vote is to revert this.

Reverted.

Introduce a StyleLayer class for the in-memory representation
of a style layer.

It handles constant resolution, paint property cascading, and
paint property calculation. The paint functions now accept a
layer as one of the parameters and read all layer properties from
it. This simplifies the rendering flow quite a bit.

The concept of a "bucket" is now something that only the workers
need to know about. The main thread does not create buckets, it
merely stores the buffers and elementGroups on the tile when they
are received from the worker.
_cascade ⇢ _resolve (called automatically)
_cascadeClasses ⇢ _cascade (called when classes change)
recalculate ⇢ _recalculate (it's internal-only)
This is a blend of #901 and #939.

The format for featuresAt results changed again. Instead of
result-per-geometry-cross-layer, each result has a `layers`
array with all layers that contain the feature. Thus avoiding
duplication of geometry and properties in the result set.
This can happen if the referent layer is skipped due to min/maxzoom.
Conflicts:
	js/render/draw_line.js
	js/style/style.js
	package.json
@jfirebaugh
Copy link
Contributor

@ansis I merged master here but rendering tests are currently failing. Can you take a look?

@jfirebaugh
Copy link
Contributor

This will need to be merged to master manually (it's targeting mb-pages, which is no longer correct).

@ansis
Copy link
Contributor Author

ansis commented Jan 22, 2015

@jfirebaugh all render tests are passing for me. Which ones are failing for you? up to date test-suite?

@jfirebaugh
Copy link
Contributor

I'm seeing the same error as the Travis build:

# geojson default
/home/travis/build/mapbox/mapbox-gl-js/node_modules/tape/index.js:75
        throw err
              ^
TypeError: Cannot call method 'join' of undefined
    at LineAtlas.getDash (/home/travis/build/mapbox/mapbox-gl-js/js/render/line_atlas.js:21:25)
    at Object.drawLine [as line] (/home/travis/build/mapbox/mapbox-gl-js/js/render/draw_line.js:57:37)
    at GLPainter.drawLayers (/home/travis/build/mapbox/mapbox-gl-js/js/render/painter.js:255:25)
    at GLPainter.drawTile (/home/travis/build/mapbox/mapbox-gl-js/js/render/painter.js:241:10)
    at Object.exports._renderTiles [as render] (/home/travis/build/mapbox/mapbox-gl-js/js/source/source.js:58:17)
    at GLPainter.render (/home/travis/build/mapbox/mapbox-gl-js/js/render/painter.js:231:20)
    at util.extend.render (/home/travis/build/mapbox/mapbox-gl-js/js/ui/map.js:329:22)
    at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)

@ansis
Copy link
Contributor Author

ansis commented Jan 22, 2015

my spec was not up to date. Test fixed in 34fc5b4

@jfirebaugh
Copy link
Contributor

Merged!

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.

5 participants