#1688 closed defect (fixed)
Auto Angle - Incorrectly rotated Labels
Reported by: | Owned by: | jmckenna | |
---|---|---|---|
Priority: | high | Milestone: | 6.0 release |
Component: | Documentation - MapServer | Version: | 4.6 |
Severity: | normal | Keywords: | |
Cc: | sdlime, woodbri@…, chodgson@…, dmorissette |
Description (last modified by )
In each case where a label was angled incorrectly, it was angled incorrect by roughly +- 180°.
Versions 4.6.2 is affected as well as the CVS head (feb. 24, 2006).
The problem is with ANGLE AUTO. More precisely in msPolylineLabelPoint() in mapprimititve.c:
if(p->line[i].point[j-1].y < p->line[i].point[j].y){ *angle = (270.0 - (MS_RAD_TO_DEG*theta)); // *angle = (90.0 - (MS_RAD_TO_DEG*theta));<-----------------BEFORE (WRONG) }else{ *angle = -(270.0 - (MS_RAD_TO_DEG*theta)); //*angle = -(90.0 - (MS_RAD_TO_DEG*theta));<-----------------BEFORE (WRONG)
Attachments (11)
Change History (54)
comment:2 by , 18 years ago
attachments.isobsolete: | 0 → 1 |
---|
comment:5 by , 18 years ago
Cc: | added |
---|
I've tried the patch. It defeats the code that tries to keep text upright at all angles. I'll attach the angle test case that shows the effect for many angles pre and post patch. I'm not sure this is a good change. CC'ing Steve Woodbridge and Daniel for more comments. Steve
comment:6 by , 18 years ago
attachments.mimetype: | text/plain → image/gif |
---|
comment:7 by , 18 years ago
I think that this problem is because the characters are being rendered separately and there is no way to independently control the the "UP" sense of the individual characters. Since AUTO angle text in mapserver is controlled to keep it always up based on the example test that Steve posted, there is no real way to coordinate multiple random character with respect to "UP". If there were a semantic that said for example this characters are part of a grouped label (and they should be draw together or not at all)? then we might be able to coordinate "UP" for the grouping similar to the way we do for labels follow path. Another possiblity might be to apply this patch as another labeling ANGLE mode, like ANGLE ESRI or something to maintain compatibility with that product.
comment:8 by , 18 years ago
I like the idea of a ANGLE ESRI option, or something along that line. We are trying to run comparable WMS services with Mapserver and ArcIMS using the same data. This would be a useful feature.
comment:9 by , 18 years ago
I thought about the new option last night too, it's probably the best solution. I don't think we can use ESRI as a keyword though. Other ideas? Something like: ANGLE AUTOTRUE ANGLE AUTO2 ????? Steve
comment:11 by , 18 years ago
So the question is "what is up". It appears that the original code words fine for Steve's example of features whose labels have > 1 characters. Whereas, the proposed solution works well with features whose lables have only 1 character. Thus, Why not modify the msPolyLineLabelPoint function to behave differently depending upon how long the label is, by means of modifying its signature to take any extra field: int length Then add another if statement that does the original angle calculation on length != 1, and the newly proposed calculations for length = 1. Then modify the call in mapdraw.c to pass in the label's character length when it calls msPolyLineLabelPoint. One could make the following assumptions: the user is either rending their label with one character per feature comprising the name (made up of multiple features), or they are rendering their label with the entire feature name contained within one feature.
comment:12 by , 18 years ago
I think we should let the map file creator decide the behavior. Then we don't need to make assumptions in the code.
comment:13 by , 18 years ago
I can't see why we'd want to treat 1 character labels any differently than 2 character labels. Upside down or rightside up may be important in both cases. There are other instances when 1 character-wide strings are used commonly. I'm thinking specifically of truetype lines. Come to think of it there's a precedence to use "true" angles when orienting features along a line with that feature. I'll have to check on that. Steve
comment:14 by , 18 years ago
I don't like the idea of behavior changing based on length, this could cause side-effects for legit 1-character labels that we want to keep oriented "UP". It seems that what we really need is a way to tell MapServer in the mapfile that in this specific case we want to orient the label according to the direction of the line segment and not try to be smart about adjusting the angle to keep it "UP". I agree with the suggestion of a new ANGLE <something> keyword, but not ESRI for sure, and AUTOTRUE doesn't say much about what it does and how it's different from AUTO... might as well use AUTO2 then and document the various algorithms in the docs, that would leave room for AUTO3 and so on at a later time. Or something more meaningful like "ANGLE FOLLOWSEGMENT" ... but I think Steve wrote before that he preferred short keywords.
comment:15 by , 18 years ago
I think that the control needs to be in the mapfile also, but that does not answer the question of what is "UP". "UP" for the purpose of multichar strings is is such the if the string follows a line vector it is up based on the radial spokes test case the Steve and I came up with for testing, which I think generally implies that text should be oritented to be moving from left to right and should be oriented for up as up on the mapimage. For strings that follow a path of a multisegmented polyline the bulk of the characters should appear right-side up. For single characters from ESRI, I think we are trying to define a scheme that matches their orientation of UP.
comment:16 by , 18 years ago
Let me see how the ttf lines handle toggling the option and I'll report back. Steve
comment:17 by , 18 years ago
Just wondering if there have been any new and exciting developments regarding this labeling issue. Thanks, Ben
comment:18 by , 18 years ago
Looks like the ball was in my court. The truetype polylines use the GAP symbol parameter to toggle the angle handling. That won't work in the case obviously so let's just pick a ANGLE keyword... Steve
comment:19 by , 18 years ago
It is hard to find a word that is descriptive of the algorithm. I suppose AUTO2 is just as meaningful as anything else, so long as the algorithms are documented. Does AUTO2 suit everyone?
comment:20 by , 18 years ago
I think that's fine for now. Since this will be a development version feature addition we have time to tweak if necessary. Steve
comment:21 by , 18 years ago
Cc: | added |
---|
comment:22 by , 18 years ago
Well, it looks like I'm taking up this 'bug' and trying to get it resolved. My first question is, is this large enough to warrent an RFC? Looking throught RFC1, it doesn't seem to fit the need, but since there is a change to the mapfile, if feels to me that there should be greater visibility than in this bugtracker. Any thoughts?
by , 18 years ago
Attachment: | auto2.tar.gz added |
---|
Proposed patch to implement an alternate auto-angle method
comment:23 by , 18 years ago
Only thing I worry about looking at the patch is setting an angle via MapScript. Something like $label->{angle} = $mapscript::MS_AUTO2; won't work. Of course we can set autoanglealgorithm in mapscript but it's quite different from how things would work in the mapfile. That's the case now with ANGLE AUTO. Perhaps this is the time (5.0 I mean) when we should ditch parameters autofollow and autoangle altogether and go with angle taking either a double or matching some constants MS_ANGLE_AUTO, MS_ANGLE_AUTO2 or MS_ANGLE_FOLLOW... Those constants would have to be odd values unlikely to be used as real angles (99990, 99991, 99992) or something like that. This would allow the new feature plus fix MapScript set/get... The patch would largely be fine, we'd just change how label->angle is interpreted throughout the code. Steve
comment:24 by , 18 years ago
I hadn't looked as closely as I should have at the mapscript implications. We passed this new issue past the client, and they aren't too keen on having that large a change done just now, though possibly in the future. I prefer your fix. It's cleaner and more consistent throughout mapserver, but without a contract, I can't commit the time to it right now. Is there a shorter way to improve the consistency, without touching so much code? Is anyone able to help out with testing and/or advice to help me pull this off a bit faster? I'm afraid my knowledge of the mapserver internals isn't too far reaching.
comment:25 by , 18 years ago
I should know better than question why I did things one particular way in the first place. There was usually a reason. Not always a good one, but a reason none the less. Turns out that label->angle is very dynamic and holds computed angles in the case of auto angle computation. That means the patch is probably ok although we should think about MapScript a bit more. My idea won't work... Steve
comment:26 by , 16 years ago
I would like to encourage capable people to fix this bug in the near future, if I may be so bold. Seems to be pending for a while and imho it creates a lot of unnecessary work-around-jobs.
comment:27 by , 16 years ago
Description: | modified (diff) |
---|---|
Milestone: | → 5.2 release |
comment:28 by , 16 years ago
Description: | modified (diff) |
---|
Here is an idea: instead of adding a new autoanglealgorithm member in the labelObj as was proposed in the auto2.tar.gz patch, we could get rid of autoangle and autofollow and combine all of them into a single label->anglemode member with the following possible values:
- 0, the default means that the angle value is either static or bound to an attribute
- MS_AUTO
- MS_AUTO2 (the new algorithm)
- MS_FOLLOW
Steve, do you expect any problem with that approach? I can take this ticket if you want, just reassign to me.
comment:29 by , 16 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Dan, I think that would work. MapScript would be simplified ($label->anglemode = ...) and I assume we'd keep the mapfile syntax the same (e.g. ANGLE AUTO2). We could allow explictly setting the anglemode in the mapfile as well with the idea of depricating ANGLE AUTO sometime in the distant future if necessary.
I'll gladly reassign... ;-)
Steve
comment:31 by , 15 years ago
Milestone: | 5.4 release → 6.0 release |
---|
comment:32 by , 14 years ago
Cc: | added |
---|---|
Owner: | changed from | to
by , 14 years ago
Attachment: | labe-error-trunk.gif added |
---|
same test case with current trunk 26/01/10
comment:33 by , 14 years ago
Status: | new → assigned |
---|
As you can see, with the current trunk, there is only one label that is not ok. I'm going to investigate in this bug.
by , 14 years ago
Attachment: | label-error-trunk.gif added |
---|
Forget labe-error-trunk.gif image, this is the proper one
by , 14 years ago
Attachment: | label-error-trunk-with-patch.gif added |
---|
Same trunk image with the patch applied
comment:34 by , 14 years ago
Ok, after a few tests, I understand better the issue (you can ignore my last comment). Since the ticket is relatively old, I will repeat two basic things to be clear:
- The shapefile in the test case use 1 feature per letter.
- Some features are simple lines with two points that are not vertical.. but almost if we see it from a distance.
What's happening here is that the features are relatively small (with the extent and map size used in the label-error-trunk.gif image) and when MapServer converts the lon/lat to pixels, the longitude/latitude are close enough to be represented by the same pixel value (for the x I mean). Once in the algorithm, MapServer has no idea what's the best to do if the x value are the same. if Here's the part of the code:
if(p->line[i].point[j-1].x < p->line[i].point[j].x) { /* <-- everything is decided here */ ... } else { ...
I would say we only need a mapfile label keyword to tell mapserver to force the label' sense. No need to duplicate the algorithm, just adding a flag would do the job. So the new if instruction would be:
if ((p->line[i].point[j-1].x < p->line[i].point[j].x) || ((p->line[i].point[j-1].x == p->line[i].point[j].x) && force_sense_flag)) { ...
You can check the two attached image:
- label-error-trunk.gif
- label-error-with-patch.gif
Is my understanding of the problem correct ? Do we still want to get rid of autoangle and autofollow and combine all of them into a single label->anglemode member ?
comment:35 by , 14 years ago
Cc: | added; removed |
---|
by , 14 years ago
Attachment: | auto2_chodgson.patch added |
---|
Retry of the auto2 functionality, replacing label.autoangle/label.autofollow with label->anglemode
comment:36 by , 14 years ago
I attached a patch which takes the approach of removing label.autoangle and label.autofollow with label.anglemode, and adding support for the auto2 keyword and function. I've done some basic testing, auto2 now always uses the "true" angle of the line while auto keeps the same behavior. I would appreciate a review of my changes.
Note that I updated maplexer.l appropriately and included that in the patch but I didn't include the resulting changes to maplexer.c in the patch because my lex seems to be a different enough version that it adds all kinds of non-related changes.
comment:37 by , 14 years ago
Component: | MapServer C Library → MapServer Documentation |
---|
The patch looks good and works well. I've only changed the following code:
if (anglemode != MS_NONE && (point_repeat == 1))
for
if ( ((anglemode == MS_AUTO) || (anglemode == MS_AUTO2)) && (point_repeat == 1) )
in mapprimitive.c to keep the expected behavior.
Thanks a lot for your work chodgson!
Fixed and committed in r10021. The PHP/Swig mapscript documentation should be updated accordingly.
comment:38 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:39 by , 14 years ago
== MS_AUTO2 was because I'm not sure about the fall-back situation - there are supposedly some case where the MS_FOLLOW case should fall-back to the MS_AUTO behavior. Previously, everywhere that autofollow was being set to true, so was autoangle, so I figured that it might make sense to accept MS_FOLLOW as the same as MS_AUTO... there may have been other cases where I should have done the same but the whole fall-back case really wasn't very clear to me. |
Perhaps someone else has an a example of such a case that they can test against this patch?
comment:40 by , 14 years ago
chodgson, see http://trac.osgeo.org/mapserver/ticket/3160 to understand the expected behavior and why we only accept MS_AUTO/MS_AUTO2 in this case. Let me know if you need more explanation after.
comment:41 by , 14 years ago
I'll certainly defer to your better knowledge of this, but I'm just looking at the old lines 1405-6 in mapfile.c, and the comment on it:
label->autofollow = MS_TRUE; label->autoangle = MS_TRUE; /* Fallback in case ANGLE FOLLOW fails */
which I replaced with:
label->anglemode = MS_FOLLOW;
Because of this, any previous check for autoangle == true should accept anglemode == (MS_AUTO|MS_AUTO2|MS_FOLLOW) - to maintain the pre-existing behaviour. Of course, it's possible that we never reach this code if anglemode = MS_FOLLOW so it may be a moot point, I haven't traced the logic carefully enough to really be sure.
comment:42 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
chodgson, thanks for pointing me this. It's fixed in r10068. In the same order, I've fixed a few other things related to this problem. The changes in mapdraw.c are also important, I've seen bad behaviors in my maps without them.
Let me know if you see something else.