-
-
Notifications
You must be signed in to change notification settings - Fork 977
Implement data-driven styling for line-dasharray #5812
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
This would be a good use case for being able to form arrays dynamically: maplibre/maplibre-style-spec#950. |
…riven-dasharray # Conflicts: # package-lock.json # package.json
1f161aa to
b7ae6b9
Compare
This reverts commit d382706.
|
I've done another round of code review changes. Again, I've left open any conversations that I wasn't able to obviously resolve. |
| path: coverage-merged | ||
|
|
||
| - name: Code Coverage Annotation | ||
| if: github.event.pull_request.head.repo.full_name == github.repository |
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.
This is probably the least problematic one as if all lines are covered this wouldn't fail, are you sure this creates problems?
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 noticed that removing this if: condition in d382706 caused this step to fail.
- Our code coverage reporting and enforcement step continues to work correctly.
- The failure appears to be related to annotation permissions. Since this workflow is running from my fork, it doesn't have the necessary permissions to create annotations in the main MapLibre GL JS repository. The annotations are helpful but disabling them does not compromise our quality standards.
- Regarding the four statements marked as uncovered: these are actually covered by our render tests. I verified this by commenting them out and watching the render tests fail. This type of coverage reporting discrepancy is something we've seen before with our test setup. Addressing it would be outside this PR's scope.
Based on these factors, keeping the if: condition seems like the right approach here.
HarelM
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.
Thanks! I'll go ahead and release the spec package.
|
I've just released version 24.2.0 of the style-spec. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
I've done another round of revisions, including updating |
|
Is there a demo using this new feature anywhere? |
I don't have anything up online. Sorry! |
|
I think the only thing left is the changelog. Can you please add to it and I'll merge this PR and release a new version. |
…gl-js into data-driven-dasharray
|
Done |
* Add render test * Update line_sdf.vertex.glsl and line_program.ts * wip * Update program_configuration.ts * WIP * WIP * WIP * WIP * WIP * WIP * WIP * Point to dev version of style-spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Try another way to link dev style spec * Update program_configuration.ts * Update draw_line.ts * Update line_program.ts * Update style.json * Begin migrating to using standard property function pattern utils * Update program_configuration.ts * Update program_configuration.ts * WIP * WIP * wip * Update line_program.ts * Update draw_line.ts * BEGIN EXPERIMENT * wip * Update line_sdf.fragment.glsl * WIP * WIP * wip * WIP * test * WIP * WIP * Eliminate u_patternscale_a/b * Generalize ImagePosition * Add ImagePositionLike class * WIP * WIP * WIP * wip * Update line_sdf.fragment.glsl * multiply line pattern by 1024 * Got cross-worker line atlas management working * Self code review * Self PR review * Create separate BucketFeature#dashes property * Update bucket.ts * Make PopulateParameters#dasharrayDependencies and object * WIP * WIP * Run lint --fix * Update program_configuration.ts * Remove parameter from lineSDFUniformValues maplibre#5812 (comment) * Get rid of 1024 magic constant * Fix CI * Fix render tests * Fix tests * Remove TODO comment * Remove other TODO comment * Fix lint errors * Fix type error * Delete line-dasharray.html * Fix a couple render tests * Update program_configuration.ts * Revert "Update program_configuration.ts" This reverts commit 71efa53. * Hack to maybe make tests pass * Update line_program.ts * Factor out createBucketLineAtlas * Create entirely separate dasharray attribute s def WIP * Remove commented out code in line_sdf.fragment.glsl * Self code review * Update min.test.ts * Update worker_tile.ts * Fix tsc * Fix tests * Rename dasharray to dash * Rename dasharray to dash * Fix tests * Fix tests * Revert * Revert "Revert" This reverts commit 8e6f612. * Update 'expectedBytes' * Refactor * Refactor * Revert * Refactor CrossFadedConstantBinder#setUniform * Remove dash_pixel_ratio attributes * Factor dash logic out of pattern_bucket_features * Factor out CrossFadedBinder * Eliminate 'u_sdfgammaratio' * Update line_sdf.vertex.glsl * Update test-all.yml * Revert unnescessary changes * Update test-all.yml * Fix CI * Fix support for round line caps * Self code review * Self code review * Self code review * Keep dash position and image position separate * Remove bucket_line_atlas * Fully remove ImagePositionLike * Export DashEntry type for docs * Self code review * Self code review * Rename CrossFadedCompositeBinder to CrossFadedPatternBinder * Self code review * Self code review * Self code review * Self code review * Self code review * Self code review * Self code review * Self code review * Self code review * Update program_configuration.ts * Update line_bucket.test.ts * Add worker_tile test * Simplify worker_tile test * Simplify worker_tile test * Add style tests * Simplify line_bucket * Simplify CrossFadedBinder * Simplify GetDashesParameters redo tests * Respond to PR review * Rename Bucket#hasDependencies * Revert GH Action change * Use a for ... of loop per PR feedback * Use the Feature#dashes type as source of truth * Update line_bucket per PR review * Revert "Revert GH Action change" This reverts commit d382706. * Record * Bump style spec version * Update CHANGELOG.md * Update CHANGELOG.md --------- Co-authored-by: Claude <noreply@anthropic.com>
See also maplibre/maplibre-style-spec#1100
This PR resolves #1235 by making the
line-dasharrayproperty "data-driven", meaning it can now reference feature property values in expressions.Due to limits in the expression system, dasharrays must be specified in the style using the
["literal", [...]]syntax. Feature properties cannot directly contain array values.Example:
Benchmarks
See full results at all.pdf
Launch Checklist
Phase 1: Basic Functionality
line_sdf.vertex.glslandline_sdf.fragment.glslline_program.tsprogram_configuration.tsline-dasharrayand its associatedvaryings underattributeNameExceptionsCrossFadedConstantBinderfor line-dasharray supportProgramConfigurationfor line-dasharray supportstyle/properties.tsCrossFadedDataDrivenPropertyto ensure support for data-drivenline-dasharraydraw_line.tsprogramConfiguration.setConstantDasharrayforline-dasharrayInclude before/after visuals or gifs if this PR includes visual changes.lineDasharrayAttributeswithpatternAttributesas support for the latter is better throughout the codebaseLineAtlas#getDashPhase 2: Pre-Code Review
dash_attributes.tsCHANGELOG.mdunder the## mainsection.Phase 3: Code Review
Underway
"interpolate"expressions with arrays. (link)nullin one expression. (link)global-stateexpressions with array literals. (link)Phase 4: Post Code Review
See #5768 for similar workflow
postinstallscript hack and updatepackage.jsonto reference latest version ofmaplibre-style-spec