Speed and access when setBothDirections(true/false)

Have a look at this code:

 @Test
    public void testEncoder() {
        {
            CarFlagEncoder enc = new CarFlagEncoder().setSpeedTwoDirections(true);
            EncodingManager em = EncodingManager.create(enc);
            GraphHopperStorage graph = new GraphBuilder(em).create();
            EdgeIteratorState edge = graph.edge(0, 1, 10, false);
            // if setTwoDirections(true) access is blocked and speed set to zero for one of the directions
            assertTrue(edge.get(enc.getAccessEnc()));
            assertFalse(edge.getReverse(enc.getAccessEnc()));
            assertEquals(60, edge.get(enc.getAverageSpeedEnc()));
            assertEquals(0, edge.getReverse(enc.getAverageSpeedEnc()));
            assertEquals(0.6, new FastestWeighting(enc).calcEdgeWeight(edge, false));
            assertEquals(Double.POSITIVE_INFINITY, new FastestWeighting(enc).calcEdgeWeight(edge, true));
        }
        {
            CarFlagEncoder enc = new CarFlagEncoder().setSpeedTwoDirections(false);
            EncodingManager em = EncodingManager.create(enc);
            GraphHopperStorage graph = new GraphBuilder(em).create();
            EdgeIteratorState edge = graph.edge(0, 1, 10, false);
            // if setTwoDirections(false) access is blocked but speed is the same for both directions
            assertTrue(edge.get(enc.getAccessEnc()));
            assertFalse(edge.getReverse(enc.getAccessEnc()));
            assertEquals(60, edge.get(enc.getAverageSpeedEnc()));
            // attention: the speed is not set to zero here (compared to the case above)
            assertEquals(60, edge.getReverse(enc.getAverageSpeedEnc()));
            assertEquals(0.6, new FastestWeighting(enc).calcEdgeWeight(edge, false));
            // ... and this yields to a finite weight here!
            assertEquals(0.6, new FastestWeighting(enc).calcEdgeWeight(edge, true));
        }
    }

Is this how it should be? Its a bit inconsistent, because the speeds are different depending on setSpeedTwoDirections. It somewhat makes sense because obviously if speedTwoDirections is false we cannot set the speed to zero for one direction. But on the other hand this means that the result of FastestWeighting#calcEdgeWeight depends on this flag and we get a finite weight even for the blocked direction. Maybe the solution to this problem will be checking the access flags inside the weighting, but I am wondering why the speed is even set to zero in the first case. I think it would be more intuitive if graph.edge(0, 1, 10, false) only set the access flags (not the speeds)? But since this method has 1800+ usages its probably not going to happen :slight_smile:

Yes, this decoupling of access and speed is a big problem and has led to many problems in the past:


or https://github.com/graphhopper/graphhopper/issues/1234

So the plan is to “mentally” migrate this and use “calcEdgeWeight==Infinity” (https://github.com/graphhopper/graphhopper/issues/1835) instead of the access check. Still inside calcEdgeWeight the faster (?) accessEnc could be used.

I think it would be more intuitive if graph.edge(0, 1, 10, false) only set the access flags (not the speeds)? But since this method has 1800+ usages its probably not going to happen :slight_smile:

Even access flags seems wrong - because access of what vehicle then? car? bike?

I think we have to remove this method somehow or at least move it out of the graph so that we can remove encodingManager.flagsDefault and further dissolve the FlagEncoders. Will investigate it.

Yes, very true. The method is used frequently in tests and its quite useful in my opinion, but maybe we can move it into some object that contains a single encoder and a graph (most of these tests use a single encoder anyway).

Powered by Discourse