Opened 18 years ago

Closed 14 years ago

Last modified 13 years ago

#1688 closed defect (fixed)

Auto Angle - Incorrectly rotated Labels

Reported by: bbrehmer@… 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 dmorissette)

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)

Label_Error_ArcMap_view.jpg (24.0 KB ) - added by bbrehmer@… 18 years ago.
View from ArcMap
Label_Error_Mapserver_View.jpg (17.9 KB ) - added by bbrehmer@… 18 years ago.
View from mapserver
Label_Error.zip (3.8 KB ) - added by bbrehmer@… 18 years ago.
Mapfile and Shapefile to reproduce error.
Label_Error.2.zip (4.1 KB ) - added by bbrehmer@… 18 years ago.
Mapfile and Shapefile to reproduce error.
angles.gif (70.5 KB ) - added by sdlime 18 years ago.
Angles test case pre-suggested change
angles_1688.gif (70.5 KB ) - added by sdlime 18 years ago.
Angles test case after the suggested change
auto2.tar.gz (2.7 KB ) - added by mark@… 17 years ago.
Proposed patch to implement an alternate auto-angle method
labe-error-trunk.gif (4.7 KB ) - added by aboudreault 14 years ago.
same test case with current trunk 26/01/10
label-error-trunk.gif (2.9 KB ) - added by aboudreault 14 years ago.
Forget labe-error-trunk.gif image, this is the proper one
label-error-trunk-with-patch.gif (2.9 KB ) - added by aboudreault 14 years ago.
Same trunk image with the patch applied
auto2_chodgson.patch (18.7 KB ) - added by chodgson 14 years ago.
Retry of the auto2 functionality, replacing label.autoangle/label.autofollow with label->anglemode

Download all attachments as: .zip

Change History (54)

comment:1 by sdlime, 18 years ago

Can you post some image samples? 

Steve

by bbrehmer@…, 18 years ago

Attachment: Label_Error_ArcMap_view.jpg added

View from ArcMap

by bbrehmer@…, 18 years ago

View from mapserver

by bbrehmer@…, 18 years ago

Attachment: Label_Error.zip added

Mapfile and Shapefile to reproduce error.

by bbrehmer@…, 18 years ago

Attachment: Label_Error.2.zip added

Mapfile and Shapefile to reproduce error.

comment:2 by bbrehmer@…, 18 years ago

attachments.isobsolete: 01

comment:3 by sdlime, 18 years ago

Are you rendering just single characters?

Steve

comment:4 by bbrehmer@…, 18 years ago

Each character is rendered independently

comment:5 by sdlime, 18 years ago

Cc: woodbri@… dmorissette@… 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

by sdlime, 18 years ago

Attachment: angles.gif added

Angles test case pre-suggested change

by sdlime, 18 years ago

Attachment: angles_1688.gif added

Angles test case after the suggested change

comment:6 by sdlime, 18 years ago

attachments.mimetype: text/plainimage/gif

comment:7 by woodbri@…, 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 banders@…, 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 sdlime, 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:10 by banders@…, 18 years ago

AUTOTRUE is pretty good.

comment:11 by cjohnson@…, 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 banders@…, 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 sdlime, 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 dmorissette, 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 woodbri@…, 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 sdlime, 18 years ago

Let me see how the ttf lines handle toggling the option and I'll report back.

Steve

comment:17 by bbrehmer@…, 18 years ago

Just wondering if there have been any new and exciting developments regarding
this labeling issue.

Thanks,

Ben

comment:18 by sdlime, 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 banders@…, 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 sdlime, 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 mark@…, 17 years ago

Cc: mark@… added

comment:22 by mark@…, 17 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 mark@…, 17 years ago

Attachment: auto2.tar.gz added

Proposed patch to implement an alternate auto-angle method

comment:23 by sdlime, 17 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 mark@…, 17 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 sdlime, 17 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 paull, 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 dmorissette, 16 years ago

Description: modified (diff)
Milestone: 5.2 release

comment:28 by dmorissette, 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 sdlime, 16 years ago

Cc: sdlime added
Owner: changed from sdlime to dmorissette

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:30 by sdlime, 16 years ago

Milestone: 5.2 release5.4 release

Moving to 5.4...

Steve

comment:31 by dmorissette, 15 years ago

Milestone: 5.4 release6.0 release

comment:32 by dmorissette, 14 years ago

Cc: dmorissette added
Owner: changed from dmorissette to aboudreault

by aboudreault, 14 years ago

Attachment: labe-error-trunk.gif added

same test case with current trunk 26/01/10

comment:33 by aboudreault, 14 years ago

Status: newassigned

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 aboudreault, 14 years ago

Attachment: label-error-trunk.gif added

Forget labe-error-trunk.gif image, this is the proper one

by aboudreault, 14 years ago

Same trunk image with the patch applied

comment:34 by aboudreault, 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:

  1. The shapefile in the test case use 1 feature per letter.
  2. 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 chodgson, 14 years ago

Cc: chodgson@… added; banders@… mark@… bbrehmer@… removed

by chodgson, 14 years ago

Attachment: auto2_chodgson.patch added

Retry of the auto2 functionality, replacing label.autoangle/label.autofollow with label->anglemode

comment:36 by chodgson, 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 aboudreault, 14 years ago

Component: MapServer C LibraryMapServer 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 aboudreault, 14 years ago

Owner: changed from aboudreault to jmckenna
Status: assignednew

comment:39 by chodgson, 14 years ago

The reason for != MS_NONE instead of == MS_AUTO
== 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 aboudreault, 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 chodgson, 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 aboudreault, 14 years ago

Resolution: fixed
Status: newclosed

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.

comment:43 by aboudreault, 13 years ago

Note that the change in r10068 (mapprimitive.c) has been reverted in r10837. A bad behavior has been discovered in #3648. After a few tests, with a mapserver at rev 9416, a label with ANGLE FOLLOW was effectively not passing in that part of the code.

Note: See TracTickets for help on using tickets.