Opened 18 years ago

Last modified 13 years ago

#1688 closed defect

Auto Angle - Incorrectly rotated Labels — at Version 27

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

Change History (34)

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@…, 18 years ago

Cc: mark@… added

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

Attachment: auto2.tar.gz added

Proposed patch to implement an alternate auto-angle method

comment:23 by sdlime, 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 mark@…, 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 sdlime, 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 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
Note: See TracTickets for help on using tickets.