Elevators and Edge Types
Matt Conway
I've made quite a bit of progress on implementing elevator support; decomposing nodes by level was much easier than I thought it to be thanks to OTP's well-organized code.
I've come to an issue, though, where I'm not sure how to succeed. Novalis had suggested implementing a new edge type for elevators, which I did. (The logic behind this is that ElevatorEdges need to generate their own narratives, &c.).
The issue is when graph_builder converts the standard graph to an edge-based one; StreetUtils expects the graph to composed exclusively of PlainStreetEdges, and throws an exception when it is not ( http://pastebin.com/DbajfVMe if anyone's interested, the gist is that it can't cast my new edge type to the expected PlainStreetEdge).
Do I need to add the ElevatorEdges after converting the graph to edge based, or do I need to add PlainStreetEdges and then convert them to ElevatorEdges after edge-basing, or something else?
Thanks for all the help so far,
Matt
David Turner
Do you need help understanding that code?
>
Andrew Byrd
Maybe elevator edges should just be special cases of / a subclass of
street edges. While revising the class hierarchy I was already
considering giving roundabout and stair edges their own classes.
-Andrew
Matt Conway
I think what we need here is a way to have multiple types of EdgeNarrative for a given PlainStreetEdge because the verbs are different (e.g., down the stairs, up the elevator, around the roundabout). Then again, perhaps simply adding up and down to standard arsenal of directions would be wise; places with significant slope are often described to people as up and down (UC Berkeley, for example, has Uphill and Downhill buses - easier for people to grasp than East and West (which is what they are) or Inbound and Outbound - is it referenced to the university, or to Shattuck Ave?)
To read what you said quite literally, if I create a class ElevatorEdge extends PlainStreetEdge, will the error go away (i.e., if it's expecting a PlainStreetEdge and I pass it a subclass, will it complain?). Sorry, I'm fairly new to Java. If it works, that's probably the best of both worlds because it gives us a clear separation between streets and things like them (elevators are more like streets than like transit, by virtue of the lack of any meaningful schedule; for all intents and purposes they can be traversed at any time).
-Matt
David Turner
> To read what you said quite literally, if I create a class
> ElevatorEdge extends PlainStreetEdge, will the error go away (i.e., if
> it's expecting a PlainStreetEdge and I pass it a subclass, will it
> complain?). Sorry, I'm fairly new to Java. If it works, that's
> probably the best of both worlds because it gives us a clear
> separation between streets and things like them (elevators are more
> like streets than like transit, by virtue of the lack of any
> meaningful schedule; for all intents and purposes they can be
> traversed at any time).
Don't do that.
Yes, from a Java perspective, it would work there.
But it wouldn't work overall, because you would lose your elevator-ness
in the conversion to an edge-based graph.
Matt Conway
elevators accurately. The way I'm thinking of modeling them now is as
vertical edges; a multi story elevator is an edge from floor 0->1, then
1->2, the 2->3 and so on. There is a fairly high cost for getting on an
elevator (much as there is for transit, but we were modeling this as a
streetish type) but the cost for riding an additional floor is very
small. I'd be concerned that, if we assign a fairly high cost to
elevators for simplicity's sake, we'll either
a) have the Trip Planner avoiding elevators in skyscrapers, because a
20-story elevator ride would be costed as 20 times a 1 story ride (which
it clearly is not), or
b) hopping back and forth between the elevator and the stairs, depending
on how far away the door to the stairwell is from the elevator on each
level.
I'm wondering if we need a more complex graph structure (shown here in
side view):
+--+-------
|
+--+-------
|
+--+-------
With + being a vertex, ------- being the way that comes in from the
side, -- being a short edge that is costly to traverse forwards but
inexpensive to traverse backwards (i.e., costly to board an elevator but
not to get off), and | being an inexpensive edge that represents one
level of an elevator.
Thoughts?
Thanks,
Matt
On 01/06/2012 09:09 AM, Matt Conway wrote:
> Would you prefer I change makeEdgeBased or make this a special type of
> PlainStreetEdge? It looks like Andrew has already extended
> PlainStreetEdge for stairs, so it seems logical to o the same for
> elevators.
>
> Thanks again for all your help,
> Matt
David Turner
(more-or-less). So I say go for it.
Matt Conway
makeEdgeBased to support multiple edge types, or modify PlainStreetEdge
to support elevators? I'm leaning towards #2 because it makes it more
flexible for future expansion (I can see us ending up with a lot of
different things in PlainStreetEdge; there's basically three cases
already for elevators—the board edge, the in-transit edge and the alight
edge—and there are other edge types where the costing infrastructure in
PlainStreetEdge might not work). The costing structure in
PlainStreetEdge won't work for elevators because they have no dimension,
so multiplying by an elevatorReluctance will always give 0.
Then again, I'm having a bit of a hard time wrapping my head around
makeEdgeBased and how I would even extend it to support multiple edge
types. I think I would need two edge types for each edge, right? One
for before conversion and one for after?
I'm trying to code this in the least disruptive way possible, since
elevators are only a very small part and will not even be present in
many graphs.
-Matt
David Turner
> There was also one other question buried in there: should I extend
> makeEdgeBased to support multiple edge types, or modify PlainStreetEdge
> to support elevators?
Modify makeEdgeBased. Otherwise, it will make TurnEdges out of your
Elevators, and that would be sad.
> Then again, I'm having a bit of a hard time wrapping my head around
> makeEdgeBased and how I would even extend it to support multiple edge
> types.
These resources might make it clearer:
https://github.com/openplans/OpenTripPlanner/wiki/GraphStructure
http://en.wikipedia.org/wiki/Line_graph
> I'm trying to code this in the least disruptive way possible, since
> elevators are only a very small part and will not even be present in
> many graphs.
Don't worry too much about disruption; we've found that these things
tend to get reused in various ways.
Matt Conway
I'm still having a little trouble wrapping my head around this. I
understand the concepts of the line-based and edge-based graphs, and I'm
beginning to understand how turn restrictions are built in
StreetUtils.java. I have a few questions and a proposal:
a) does any new edge type that is added through OpenStreetMap need to
have a vertex type for it to be converted to after the graph is made
edge-based?
b) what type of edge should be used to link vertices that are not
streets (e.g., elevators)? TurnEdges are currently used for streets, can
I re-use them, should I use DirectEdge or do I need a new edge?
And the proposal: in order to add support for multiple edge types coming
from OpenStreetMap (and I guess this applies to shapefiles as well), we
modify makeEdgeBased and move the bulk of the work into class methods,
so that edges handle converting themselves into vertices and building
turn edges to their counterparts. Then, as long as an edge implemented
makeEdgeBased, it wouldn't matter what edge types were put into the
initial graph, and StreetUtils.makeEdgeBased could look like
for (Edge e : edges) {
e.makeEdgeBased();
}
My concern is that it's a very large change because it goes from looping
through all of the original vertices in the graph to looping through the
edges, and would need to be extensively tested with OSM and Shapefiles,
which may be more time than I can commit. The other option is to just
wrap makeEdgeBased's core code in an if (e instanceof Whatever) ...
else if (e instanceof Whatever), but that's a little ugly.
-Matt
Andrew Byrd
> a) does any new edge type that is added through OpenStreetMap need to
> have a vertex type for it to be converted to after the graph is made
> edge-based?
> b) what type of edge should be used to link vertices that are not
> streets (e.g., elevators)? TurnEdges are currently used for streets, can
> I re-use them, should I use DirectEdge or do I need a new edge?
My comment about subclassing street edges was misguided. As you pointed
out, the cost model is all wrong. Elevators are more like transit, with
a fixed cost for boarding and a low cost per "stop" once on board. I
agree with David that elevators should be handled like vertical subways,
and your diagram is very similar to the way we do transit.
In that case, you want makeEdgeBased to not touch the vertices and edges
that represent being on-elevator. The line-graph conversion is not
applied to transit because we don't need turn restrictions there.
I can see two ways to do this.
1. Generally allow transit-like structures to be present and connected
into the street graph during conversion. makeEdgeBased currently assumes
that it is going to see only PlainStreetEdges (it casts all the edges it
encounters to this type). One way of changing that (off the top of my
head, could probably be improved upon): I would allow one other type of
zero-cost edge (FreeEdge) to be present at EndpointVertices, and when a
FreeEdge is spotted as the incoming or outgoing edge of a vertex
undergoing conversion, rather than calling getStreetVertexForEdge on it,
I would take its existing from or to vertex as the endpoint for
generated edges. Example before conversion:
F B
: |
G<<E==A--D
: |
H C
: ElevatorHop
< ElevatorBoard/Alight
= FreeEdge
| PlainStreetEdge
E is the "Elevator Stop", F G H are on the elevator, A B C D are of type
EndpointVertex. When converting A you would get the 6 expected turn
edges between the 3 adjacent streets, but also 3 turnEdges from BA/CA/DA
to E, and 3 FreeEdges between E and the adjacent StreetVertices around A.
As long as they were composed of something other than PlainStreetEdges
and attached to the street network with FreeEdges, transit-like
structures could be ignored without becoming detached from the turn-graph.
2. Totally detach the elevator structure from the streets (no edges
between them). Make sure the on-elevator vertices are not passed in to
makeEdgeBased. If E is of type TransitStop, the street/transit linker
will connect it to the street network like any other transit. The
problem here is that the elevator stops are all piled on top of each
other vertically. You would need some mechanism for linking to only ways
at the appropriate height. This amounts to throwing away the
street/elevator connectivity information in OSM guessing it back into
existence, so I'm including option 2 just to underscore that this
seemingly simple option would probably get ugly.
I am reworking the edge/vertex class hierarchy such that there are more
specific categories. If you just make a new ElevatorBoardEdge,
ElevatorAlightEdge, and ElevatorHopEdge with traverse methods that apply
your custom costs, I will merge them in later as types of transit edge.
> And the proposal: in order to add support for multiple edge types coming
> from OpenStreetMap (and I guess this applies to shapefiles as well), we
> modify makeEdgeBased and move the bulk of the work into class methods
I see where you're coming from (I have a general preference for instance
methods attached to the objects they affect/reference the most), but it
will probably be easier to get it working as-is and refactor later if it
ends up looking ugly.
-Andrew
Matt Conway
Thanks for the input; this is really clear. I also prefer option #1.
I took a stab at a refactor of makeEdgeBased trying to convert it to instance methods, the code is at https://github.com/mattwigway/OpenTripPlanner/commit/a8f8d203b0268807e82577dec06df3b943de9ba1 and
https://github.com/mattwigway/OpenTripPlanner/commit/fb685970d6451c39b0d75b413a0dc9bdd636a60c. It still depends on casts (removing that requires defining a few more interfaces/superclasses); I'm not sure if I should continue to develop the code here or if I should revert to the old makeEdgeBased and make the modifications you suggested. What are your thoughts on the matter?
I don't really have a strong lean here; I think a lot of it depends on how this will interact with your Edge/Vertex refactor.
Oh, and in my commits there may be an errant tab here or there (initially didn't have Emacs configured correctly), I'll make sure those go away before submitting a patch.
-Matt
Matt Conway
Matt Conway
My next question is, how do I set up costing and narrative generation on the new edge types? It looks like costing is done in traverse() and I can probably figure that out from PlainStreetEdge, but I can't find any examples of edges implementing EdgeNarrative for narrative generation.
-Matt
David Turner
On Mon, 2012-01-09 at 20:20 -0800, Matt Conway wrote:
> OK, I've got it building believable graphs now; anything connected by
> FreeEdges is left unchanged in the final graph.
>
> My next question is, how do I set up costing and narrative generation
> on the new edge types? It looks like costing is done in traverse() and
> I can probably figure that out from PlainStreetEdge, but I can't find
> any examples of edges implementing EdgeNarrative for narrative
> generation.
We get that via DirectEdge, so actually most of the edge types do
implement EdgeNarrative.
Matt Conway
they should be user-configurable, but I'm not sure exactly how to do that.
Thanks,
Matt
> Delivered-To: mattw...@gmail.com
> Received: by 10.231.223.193 with SMTP id il1cs36729ibb;
> Mon, 9 Jan 2012 20:29:19 -0800 (PST)
> Return-Path:<opentripplanner-dev+bncCKf...@googlegroups.com>
> Received-SPF: pass (google.com: domain of opentripplanner-dev+bncCKf...@googlegroups.com designates 10.68.211.131 as permitted sender) client-ip=10.68.211.131;
> Authentication-Results: mr.google.com; spf=pass (google.com: domain of opentripplanner-dev+bncCKf...@googlegroups.com designates 10.68.211.131 as permitted sender) smtp.mail=opentripplanner-dev+bncCKf...@googlegroups.com; dkim=pass header.i=opentripplanner-dev+bncCKf...@googlegroups.com
> Received: from mr.google.com ([10.68.211.131])
> by 10.68.211.131 with SMTP id nc3mr1070690pbc.16.1326169758261 (num_hops = 1);
> Mon, 09 Jan 2012 20:29:18 -0800 (PST)
> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
> d=googlegroups.com; s=beta;
> h=x-beenthere:received-spf:message-id:subject:from:to:date
> :in-reply-to:references:x-mailer:mime-version:x-original-sender
> :x-original-authentication-results:precedence:mailing-list:list-id
> :x-google-group-id:list-post:list-help:list-archive:sender
> :list-subscribe:list-unsubscribe:content-type
> :content-transfer-encoding;
> bh=t9Euzok/zrkqLaKI8S45wBXax372WgqLSLgdHS2mueo=;
> b=aNE9Q0Cmw137BmhXy7/iF3aJn/J9525SQlJqyBu+0XsGTDM3vkwHIV3VXCG+HCKdAv
> dMQFROB5HNpcd+qS9a8Sspf0hT0hjgf4oUbJ8PfSg8EeLD1txHKo9spTi5MJQ5B/dKGf
> 4yIEWy0Bfbr3g9sb1FWMCpcqKncLmg4mYWSgQ=
> Received: by 10.68.211.131 with SMTP id nc3mr334860pbc.16.1326169750555;
> Mon, 09 Jan 2012 20:29:10 -0800 (PST)
> X-BeenThere: opentripp...@googlegroups.com
> Received: by 10.68.156.102 with SMTP id wd6ls68736206pbb.2.gmail; Mon, 09 Jan
> 2012 20:29:10 -0800 (PST)
> Received: by 10.68.196.130 with SMTP id im2mr12606258pbc.3.1326169750189;
> Mon, 09 Jan 2012 20:29:10 -0800 (PST)
> Received: by 10.68.196.130 with SMTP id im2mr12606257pbc.3.1326169750178;
> Mon, 09 Jan 2012 20:29:10 -0800 (PST)
> Received: from homiemail-a21.g.dreamhost.com (caiajhbdcbbj.dreamhost.com. [208.97.132.119])
> by gmr-mx.google.com with ESMTP id c6si29476720pbo.0.2012.01.09.20.29.06;
> Mon, 09 Jan 2012 20:29:06 -0800 (PST)
> Received-SPF: neutral (google.com: 208.97.132.119 is neither permitted nor denied by best guess record for domain of nov...@novalis.org) client-ip=208.97.132.119;
> Received: from homiemail-a21.g.dreamhost.com (localhost [127.0.0.1])
> by homiemail-a21.g.dreamhost.com (Postfix) with ESMTP id CE718300074
> for<opentripp...@googlegroups.com>; Mon, 9 Jan 2012 20:29:05 -0800 (PST)
> Received: from [18.111.74.29] (unknown [18.111.74.29])
> (using SSLv3 with cipher DHE-RSA-AES256-SHA (256/256 bits))
> (No client certificate requested)
> (Authenticated sender: nov...@novalis.org)
> by homiemail-a21.g.dreamhost.com (Postfix) with ESMTPSA id 8B8F9300072
> for<opentripp...@googlegroups.com>; Mon, 9 Jan 2012 20:29:05 -0800 (PST)
> Message-ID:<1326169743.15369.1.camel@chiang>
> Subject: Re: [OpenTripPlanner] Elevators and Edge Types
> From: David Turner<nov...@novalis.org>
> To: opentripp...@googlegroups.com
> Date: Mon, 09 Jan 2012 23:29:03 -0500
> In-Reply-To:<4F0BBCA1...@gmail.com>
> References:<4F06A3AC...@gmail.com> <1325835715.13305.126.camel@chiang>
> <4F06FA29...@fastmail.net> <4F072275...@gmail.com>
> <1325869645.13305.135.camel@chiang> <4F072AD1...@gmail.com>
> <4F077389...@gmail.com> <1325910631.13305.159.camel@chiang>
> <4F07D7BA...@gmail.com> <1325956123.3237.6.camel@chiang>
> <4F08BC66...@gmail.com> <4F09AB6B...@fastmail.net>
> <4F09C304...@gmail.com> <4F0A2AC1...@gmail.com>
> <4F0BBCA1...@gmail.com>
> X-Mailer: Evolution 3.2.1-
> Mime-Version: 1.0
> X-Original-Sender: nov...@novalis.org
> X-Original-Authentication-Results: gmr-mx.google.com; spf=neutral (google.com:
> 208.97.132.119 is neither permitted nor denied by best guess record for
> domain of nov...@novalis.org) smtp.mail=nov...@novalis.org; dkim=neutral
> (bad format) header.i=@novalis.org
> Precedence: list
> Mailing-list: list opentripp...@googlegroups.com; contact opentripplann...@googlegroups.com
> List-ID:<opentripplanner-dev.googlegroups.com>
> X-Google-Group-Id: 349929814275
> List-Post:<http://groups.google.com/group/opentripplanner-dev/post?hl=en_US>,<mailto:opentripp...@googlegroups.com>
> List-Help:<http://groups.google.com/support/?hl=en_US>,<mailto:opentripplan...@googlegroups.com>
> List-Archive:<http://groups.google.com/group/opentripplanner-dev?hl=en_US>
> Sender: opentripp...@googlegroups.com
> List-Subscribe:<http://groups.google.com/group/opentripplanner-dev/subscribe?hl=en_US>,
> <mailto:opentripplanne...@googlegroups.com>
> List-Unsubscribe:<http://groups.google.com/group/opentripplanner-dev/subscribe?hl=en_US>,
> <mailto:googlegroups-manage+34...@googlegroups.com>
> Content-Type: text/plain; charset="UTF-8"
> Content-Transfer-Encoding: 7bit
Matt Conway
The narrative generation isn't working the way I think it should, so I
must not have my head around something. Do I need to use the getTrip()
method?
Basically, the way I'm implementing it now is that the edge that
generates narrative is the ElevatorAlightEdge, since it's the only edge
that knows where the user is going, so I need to generate something like
"take elevator to floor 2," but I can't figure out how that fits into
EdgeNarative (the other concern is of course i18n and l10n).
My other concern with using ElevatorAlightEdges to generate narratives
is that, if OTP snaps the end point of a trip to part of the elevator
structure, narratives will not show the elevator.
-Matt
On 01/10/2012 12:18 PM, David Turner wrote:
> TraverseOptions, generally.
David Turner
On Tue, 2012-01-10 at 12:00 -0800, Matt Conway wrote:
Matt Conway
StreetEdge, EndpointVertex and TransitStop - excellent. If I add a
traverse mode, that will probably break webapp translations, will it
not? I can update the English one and probably the Spanish one, but the
others will be... broken.
The only other wrinkle is that the floor number depends on your country.
Here in the U.S., I live on the first floor, but in England and other
places, I live on the ground floor and the first floor is the floor
above me. Also, getting an absolute floor number from OSM is tricky
anyhow; for instance, a basement or tunnel is called level=-1 in OSM,
but could be labeled Basement, Lower Level, Garage, or tunnel. I'll
think about this more, if you have any brilliant suggestions let me know
(a level_name tag or suchlike might be in order).
-Matt
On 01/10/2012 03:23 PM, David Turner wrote:
> On Tue, 2012-01-10 at 12:28 -0800, Matt Conway wrote:
>> Thanks, I thought that might be it.
>>
>> The narrative generation isn't working the way I think it should, so I
>> must not have my head around something. Do I need to use the getTrip()
>> method?
> Probably not; there is no GTFS trip involved in an elevator.
>
>> Basically, the way I'm implementing it now is that the edge that
>> generates narrative is the ElevatorAlightEdge, since it's the only edge
>> that knows where the user is going, so I need to generate something like
>> "take elevator to floor 2," but I can't figure out how that fits into
>> EdgeNarative (the other concern is of course i18n and l10n).
> Narrative generation is intended to do things symbolically, as much as
> possible. It generates a tree of objects which are serialized to xml or
> json. So i18n is done on the client.
>
> You'll probably want to add a TraverseMode (I think) for elevators,
> which will help generate that narrative. And perhaps getName() could
> return the floor number?
>
>> My other concern with using ElevatorAlightEdges to generate narratives
>> is that, if OTP snaps the end point of a trip to part of the elevator
>> structure, narratives will not show the elevator.
> That should be prevented in StreetVertexIndexServiceImpl (where "should"
> might mean "already is", or might mean "you need to do it", and I don't
> off the top of my head know which).
>
Matt Conway
Thanks,
Matt
David Turner
> Yes, looks like StreetVertexIndexServiceImpl only allows StreetVertex,
> StreetEdge, EndpointVertex and TransitStop - excellent. If I add a
> traverse mode, that will probably break webapp translations, will it
> not? I can update the English one and probably the Spanish one, but the
> others will be... broken.
It's not a huge problem; we'll send an email out to -dev telling people
to update their translations.
> The only other wrinkle is that the floor number depends on your country.
> Here in the U.S., I live on the first floor, but in England and other
> places, I live on the ground floor and the first floor is the floor
> above me. Also, getting an absolute floor number from OSM is tricky
> anyhow; for instance, a basement or tunnel is called level=-1 in OSM,
> but could be labeled Basement, Lower Level, Garage, or tunnel. I'll
> think about this more, if you have any brilliant suggestions let me know
> (a level_name tag or suchlike might be in order).
If there's a tag in OSM, feel free to include the text; if not please
feel free to propose a tag to OSM!
No router will include both England and the US, so you can feel free to
make the number configurable via a property on OSMGBI.
David Turner
cleaner. I'm still not super-happy about it.
You may well need to add another state to the state machine to get good
narrative gen.
Matt Conway
The code that I think is relevant is at https://github.com/mattwigway/OpenTripPlanner/blob/elevators-broken/opentripplanner-api-webapp/src/main/java/org/opentripplanner/api/ws/PlanGenerator.java.
I'm pretty sure I haven't provided enough useful information to diagnose the problem, but I'm not really even sure what information is needed—probing questions encouraged.
-Matt
David Turner
> OK, I'm having a lot of trouble getting the front-end stuff working
> correctly; I added a state to the state machine, and I do get
> mode="ELEVATOR" in the XML output, but the Elevator legs have
> non-equal start and end coordinates, which I don't understand because
> all of the vertices are at exactly the same point (the edges
> getGeometry methods return null as they're from FreeEdge, perhaps that
> is the problem?). The elevators seem to be getting merged with the
> walk legs before them.
This is the sort of typical problem with narrative generation. To fix
it, walk through the state machine step-by-step, with as many printlns
as necessary, to ensure that state transitions happen at the correct
places.
If you think of a way to make this cleaner or more debuggable, please do
let me know, or implement.
Matt Conway
(let me know if this wouldn't work): we could have the loop over all
states accumulate states until it gets a state with a different
TraverseMode, and then call a function with the accumulated states to
build a leg. Psuedocode (but suspiciously like a Python/Javascript hybrid):
accumulatedStates = []
legs = []
lastState = null
for (state in states) {
if (lastState != null & state.TraverseMode !=
accumulatedStates[-1].TraverseMode) {
buildLeg(accumulatedStates)
accumulatedStates = []
} else {
accumulatedStates.append(state)
}
}
def buildLeg(states) {
switch (states[0].TraverseMode) {
case TraverseMode.WALK {
// build a walk leg
// ...
return leg
}
// and so on for other modes
// ...
}
}
It might be necessary to make buildLeg take two ints and an array with
all the states, which it could then slice to get the states it's working
on. I can think of situations where it might need to look ahead or
behind to figure out narrative gen. But this does eliminate the nested
case/else if business (then again, this may have been one of your
previous refactors that didn't work out).
-Matt
Matt Conway
if taking an elevator didn't make another leg but was instead integrated
into the walking directions? I suspect that's the way most people think
about elevators (for instance, I would say my trip home from work had
two walk legs and two transit legs, even though I walk to an elevator,
then walk to BART, then catch a bus and finally walk home).
-Matt
David Turner
> It might be necessary to make buildLeg take two ints and an array with
> all the states, which it could then slice to get the states it's working
> on. I can think of situations where it might need to look ahead or
> behind to figure out narrative gen. But this does eliminate the nested
> case/else if business (then again, this may have been one of your
> previous refactors that didn't work out).
Yes, that's about how it used to look. The downside was that all the
lookahead and lookbehind was hard to understand. And it was prone to
out-of-bounds errors when (say) transit was unexpectedly at the end of
the trip.
> One other thought - would it make more sense from a narrative
> standpoint if taking an elevator didn't make another leg but was
> instead integrated into the walking directions? I suspect that's the
> way most people think about elevators (for instance, I would say my
> trip home from work had two walk legs and two transit legs, even
> though I walk to an elevator, then walk to BART, then catch a bus and
> finally walk home).
Good idea!
Matt Conway
Now the bad news: I made it work by adding something to the API. I'm going to outline why I did and why I think it won't hurt anything, but other suggestions for refactoring are welcome. I added a specialDirection field to the API response, which is always accompanied by a relativeDirection of CONTINUE (for no reason other than to provide backwards-compatibility). The specialDirection field is based on an enum that allows us to define directions in the webapp other than the cardinal and relative directions. The advantage is that it allows the special instruction "Take elevator to level <whatever>" to be translated easily in locale/<lang>.js.
Since it always specifies a fallback relative direction, I think non-special-direction-aware tools will say something like "Continue on 1" instead of "Take elevator to level 1," which isn't really much worse than the way OTP behaves now (currently, it passes through elevators with no comments whatsoever). For reference, you can see a full API response at http://pastebin.com/37gv3pyx
-Matt
David Turner
> The good news: integrating with the walking directions was relatively
> painless and the directions seem much more natural this way anyhow
> (see http://www.flickr.com/photos/mattwigway/6706233005/). It also
> lets me integrate this with multiple traversemodes (some elevators
> allow cyclists, some do not, for instance).
>
> Now the bad news: I made it work by adding something to the API. I'm
> going to outline why I did and why I think it won't hurt anything, but
> other suggestions for refactoring are welcome. I added a
> specialDirection field to the API response, which is always
> accompanied by a relativeDirection of CONTINUE (for no reason other
> than to provide backwards-compatibility). The specialDirection field
> is based on an enum that allows us to define directions in the webapp
> other than the cardinal and relative directions. The advantage is that
> it allows the special instruction "Take elevator to level <whatever>"
> to be translated easily in locale/<lang>.js.
Why can't we just use relativeDirection UP or DOWN (or ELEVATOR, if
figuring out up/down is too hard)?
Matt Conway
That said, you're a (the?) lead developer here and if you want it done with relative directions I can do that too. I'm just not sure how to represent that in the locale file, since they require to instead of on.
Whenever I see something like "special", I worry about a combinatorial
explosion of special cases -- "more special", "extra special",
"adjustments to adjustments to adjustments to special," etc. (Perhaps I
just have an overactive imagination).
There will need to be extra logic in the webapp either way; this seems
like less extra logic.
On Tue, 2012-01-17 at 15:55 -0800, Matt Conway wrote:
> I suppose you could do that as well. Up and down can be very difficult
> from OSM data at least in certain cases. Elevators didn't really seem
> like directions to me, and narrative generation needs to be a bit
> different (Take elevator to ... not Elevator on ...); it would require
> some special logic in the webapp. I thought SpecialDirection seemed a
> bit cleaner, but I could change it to a RelativeDirection if you like.
David Turner
would be:
Skręć w lewo w Lincoln Pl ("turn left on Lincoln Pl")
Kontynuuj wzdłuż Degraw St ("Continue along Degraw St")
(back-translations by Google from Google Maps in Polish, but you can see
that a different word is used).
So I continue to think that relativeDirection is correct.
Andrew Byrd
> Yeah, that's true in English, but not in other languages -- in Polish
> it would be:
>
> Skręć w lewo w Lincoln Pl ("turn left on Lincoln Pl")
> Kontynuuj wzdłuż Degraw St ("Continue along Degraw St")
>
> (back-translations by Google from Google Maps in Polish, but you can
> see that a different word is used).
>
> So I continue to think that relativeDirection is correct.
I am definitely with David on this -- we should not adapt the structure
of the response object to that of the English language. Up or down is
just another direction, and if modeling it that way means an incorrect
preposition in an itinerary description, this is a sign that we
eventually need to improve our internationalization mechanism. This is a
common situation (especially with expressions involving numbers) so the
libraries exist. Slavic languages like Polish are great for comparison
because they often use cases where English would use prepositions, and
express plurals differently.
On a related point:
> Here in the U.S., I live on the first floor, but in England and other
> places, I live on the ground floor and the first floor is the floor
> above me. Also, getting an absolute floor number from OSM is tricky
> anyhow; for instance, a basement or tunnel is called level=-1 in OSM,
> but could be labeled Basement, Lower Level, Garage, or tunnel. I'll
> think about this more, if you have any brilliant suggestions let me
> know (a level_name tag or suchlike might be in order).
> -Matt
In many places the ground floor is often numbered 0 with negative
numbers for the basement floors. This is unambiguous and simple, but you
never see it in the US for example. If -1 is not an acceptable floor
name by local convention, then that should be handled in the client.
There's a good illustration on page 9 of "Crocheting Adventures with
Hyperbolic Planes":
http://www.amazon.com/Crocheting-Adventures-Hyperbolic-Planes-Taimina/dp/1568814526
And here:
http://www.elevatorbobs-elevator-pics.com/images2/Buttons/Car/bc17.jpg
-Andrew
Matt Conway
There're two places levels come from: numeric "level" tags and level maps. Both can contain text like B, or tunnel, which can't really be parsed by the client. Level maps have prose names which I think should be used even if they're in a different language, because they are presumably what the labels will be in the elevators.
Matt Conway
repo against a recent pull of master. This should allow folks to get an
idea of what I've done. It's at
https://github.com/mattwigway/OpenTripPlanner/pull/1/files. The files
that I changed the most are (in descending order)
OpenStreetMapGraphBuilderImpl.java, StreetUtils.java and
PlanGenerator.java. If I could get some feedback on the changes before
doing a pull request that'd be great. There are two notes in OSMGBI for
places that could use an extra pair of eyes.
I did use RelativeDirection to add the ELEVATOR direction, and you will
find not a trace of SpecialDirections in the code. I also added a
boolean noZeroLevels (optional) property to OSMGBI which can be set to
true or false in graph_builder.xml. This determines if positive levels
will go by the European standard of 0, 1, 2, 3, 4, 5 or the U.S.
standard of 1, 2, 3, 4, 5, 6. In any case negative levels are left as
is. The default is true, we can debate what it should be. I personally
wish we'd adopt the 0, 1, 2... notation in the US, but I'm also that
rare American who speaks in metric.
I will run the tests tomorrow, or most of them (the full integration
test won't run on my machine, Java can't get enough heap space). The
hour is advanced even in my Pacific time zone...
Thanks!
Matt
Matt Conway
space error (which I get even on master, I just don't have enough RAM).
David Turner
looks good to me (there were more indentation issues that I didn't flag,
but I assume you'll take care of those when you take care of the one
that I did).
Andrew, what do you think?
Matt Conway
Sorry it was a bit messy, I thought I'd cleaned it up better than that.
The only thing I haven't done is to parse levels like 1;2 for stairways. I'm not sure how important that is, since it would be unlikely to have an elevator directly connected to a stairway. What is your feeling on this?
Thanks,
Matt
Matt Conway
Hey David and Matt,
I plan to review these commits later today and give you some feedback.
Once everything looks good, I propose that we cut a release for people who might be depending on current OTP internals, then I merge elevators into my branch and resolve conflicts, then I merge my branch into master, so these will be the first changes in 0.5-SNAPSHOT.
-Andrew
Andrew Byrd
1. OSMGraphBuilder.buildGraph is starting to get really long, we should
eventually factor out a few methods to clarify the process.
2. I recommend the use of symbolic constants rather than hard-coded
integers to define the subranges of otp:numeric_level. They could be up
in the hundreds of millions (2^31 > 2,000,000,000), though I guess we
won't be seeing >1000 story buildings anytime soon.
3. It still seems odd to me to have different internal representations
for floor numbers depending on the country, but it does the job for now.
We can potentially change that over if we move to an i18n library with
procedural number handling.
By the way, I used the following github feature to compare your latest
commit with master. It can be used across repositories without
submitting a pull request:
https://github.com/mattwigway/OpenTripPlanner/compare/openplans:master...mattwigway:elevators-new
-Andrew
Matt Conway
> It looks good to me. A couple of comments:
>
> 1. OSMGraphBuilder.buildGraph is starting to get really long, we
> should eventually factor out a few methods to clarify the process.
>
big-ticket.
> 2. I recommend the use of symbolic constants rather than hard-coded
> integers to define the subranges of otp:numeric_level. They could be
> up in the hundreds of millions (2^31 > 2,000,000,000), though I guess
> we won't be seeing >1000 story buildings anytime soon.
or later?
>
> 3. It still seems odd to me to have different internal representations
> for floor numbers depending on the country, but it does the job for
> now. We can potentially change that over if we move to an i18n library
> with procedural number handling.
names are; in some cases those are numbers, but often they are just
words from a level map (tunnel, basement, street, garage, &c).
>
> By the way, I used the following github feature to compare your latest
> commit with master. It can be used across repositories without
> submitting a pull request:
>
> https://github.com/mattwigway/OpenTripPlanner/compare/openplans:master...mattwigway:elevators-new
Andrew Byrd
> Hmm, yeah, I suppose we could do that. Is that something I should do
> now or later?
These were really just comments, not pressing issues. I might apply this
when I do the merge.
-Andrew