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

Add new angled-point marker placement mode for lines #3782

Merged
merged 1 commit into from Oct 26, 2017

Conversation

bmharper
Copy link
Contributor

Details are inside the commit message. I have created a visual test for this, but will rather wait for feedback before sending a PR for that.
We use this kind of rendering for drawing symbols on top of water pipes - for example, when a pipe is actually a valve, or when a pipe is closed, etc. In this case, we always want just a single symbol drawn in the center of the polyline, and we want the symbol's angle to be aligned to the line.

New angled-point marker-placement mode:
angled-point

Existing point marker-placement mode:
point

@artemp
Copy link
Member

artemp commented Oct 24, 2017

@bmharper - great feature, thanks for contributing! My only concern after skimming through the code is the use of pointer logic to select angled placement. Could we use optional<double> instead ?
Please, share visual test so we can merge this PR asap, cheers!

This adds a new mode called 'angled-point' to the marker-placement modes.
The full list of modes is then:
  point, angled-point, interior, line, vertex-first, vertex-last

Angled point is identical to point, except that when placing a marker on
a line, the marker's angle is taken from the angle of the line segment.

There is another possible use of the "angled-point" concept for polygons,
and that is for placing labels on stand (aka erf) polygons. By computing
a dominant angle for a mostly rectangular polygon, this can produce quite
good results. I'm not sure whether I should implement that now, or if I
could do that later.
@codecov
Copy link

codecov bot commented Oct 26, 2017

Codecov Report

Merging #3782 into master will decrease coverage by <.01%.
The diff coverage is 37.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3782      +/-   ##
==========================================
- Coverage   61.48%   61.47%   -0.01%     
==========================================
  Files         426      426              
  Lines       22756    22761       +5     
  Branches     3254     3254              
==========================================
+ Hits        13992    13993       +1     
- Misses       7259     7263       +4     
  Partials     1505     1505
Impacted Files Coverage Δ
src/symbolizer_enumerations.cpp 100% <ø> (ø) ⬆️
include/mapnik/geom_util.hpp 0% <ø> (ø) ⬆️
include/mapnik/markers_placement.hpp 91.17% <0%> (-8.83%) ⬇️
include/mapnik/markers_placements/point.hpp 58.62% <60%> (-0.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f02a259...05849f0. Read the comment docs.

bmharper added a commit to IMQS/test-data-visual that referenced this pull request Oct 26, 2017
This is a test that accompanies the pull request mapnik/mapnik#3782
bmharper added a commit to IMQS/test-data-visual that referenced this pull request Oct 26, 2017
This is a test that accompanies the pull request mapnik/mapnik#3782
@bmharper
Copy link
Contributor Author

I've modified the code to use optional<double>, and also sent a PR for one new visual test that accompanies this new mode: mapnik/test-data-visual#56. Apologies for force pushing to this branch - I know that makes incremental reviews harder.
Thanks for the warm welcome!

@artemp artemp merged commit 62b10d8 into mapnik:master Oct 26, 2017
@bmharper bmharper deleted the angled-point-master branch November 24, 2017 09:42
@lightmare lightmare added this to the v3.1.0 milestone Jul 16, 2018
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.

None yet

3 participants