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

MINDISTANCE does not regard label bounds #5369

Closed
olt opened this Issue Jan 13, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@olt
Contributor

olt commented Jan 13, 2017

Identical labels are placed too close together. MINDISTANCE should prevent this, but msCheckLabelMinDistance only calculates the distance from the label point (center?) and does not regard the complete bounds of the label.

The attached images shows this: Kreuzeskirchstr. are two separate linestrings and both labels are rendered even with a MINDISTANCE of 100.

mindistance 100

The following image highlights a few labels that are too close together.
min distance 50

@tbonfort

This comment has been minimized.

Member

tbonfort commented Jan 13, 2017

@olt I'd be OK with merging if you can provide a computationally efficient fix checking for this

@olt olt referenced this issue Jul 20, 2017

Merged

Fix label MINDISTANCE calculation #5464

7 of 7 tasks complete
@olt

This comment has been minimized.

Contributor

olt commented Jul 20, 2017

My proposed change in #5464 (still work in progress) would buffer the bbox of the current label to draw (in msDrawLabelCache). MINDISTANCE defines the buffer size.
All labels from current label cache with the identical text (annotext) are checked if they intersect this buffered bbox.

This not a 100% solution, as it does not use the actual text path the label, but it would be more complex to buffer this polygon. However, it does use the text path polygon from the label cache. This is still quite efficient as only identical text labels are tested for intersection and intersectLabelPolygons checks the bbox first.
My non-scientific benchmarks (shp2img -c 100) showed no measurable difference, but I will do a bit more testing.

My main question is: This will change the output of existing styles that use the MINDISTANCE feature. Less labels are expected as the MINDISTANCE is now calculated from the label bounds and not the label center. Is this OK, as long as it is documented in the changelog for 7.2? (As the current behavior does not match the documentation). Should I ask on mapserver-dev?

@olt

This comment has been minimized.

Contributor

olt commented Aug 2, 2017

I've uploaded images which show how it is now possible to evenly distribute labels. https://gist.github.com/olt/39ee2c4df38bd51e8983c8db69168988
min_distance_old_x.png are images created with current master, min_distance_new_x.png with #5464
You will notice that the old version puts much more labels, as the mindistance calculated from the center requires a much larger value.

I also annotated one image that shows how the current implementation gives unexpected results.

I also did more performance tests. I did multiple runs of shp2img with -c 100, I switched between old and new version each time so that CPU throttling does affect both in the same way.
I did test runs with actual label texts and one with a fixed text string to trigger mindistance check for each label. The minimum times were similar for both versions.

Note: The largest impact in rendering is still the font rendering. I used different MINDISTANCE values so that the old and new version both render the same number of labels.

@dmorissette

This comment has been minimized.

Contributor

dmorissette commented Aug 2, 2017

My 0.02$: I like this enhancement (Thanks!) and I think it is okay for the new version to produce a different output for the same MINDISTANCE value since the new behavior is closer to what one would expect MINDISTANCE to do in my opinion. However it may be a good idea to bring this up on the -dev list to get more opinions and to make sure there is no bad surprise after the 7.2 release.

@olt

This comment has been minimized.

Contributor

olt commented Oct 9, 2017

Merged with #5464

@olt olt closed this Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment