Add vector tile and layer in debug client for AreaStops by hannesj · Pull Request #4565 · opentripplanner/OpenTripPlanner

hannesj · 2022-11-03T08:51:35Z

Summary

Currently it is not possible to visualize the coverage areas of individual flex areas. Add a vector tile layer and use it in the debug client.

TODO: We need to decide how to do configuration fir this, should we do it automatically or require the user to do it. Currently this requires the vector tile sandbox feature to be enabled and the following layer to be included.

EDIT: Now always enabled

  "vectorTileLayers": [
    {
      "name": "debugAreaStops",
      "type": "AreaStop",
      "mapper": "DebugClient",
      "cacheMaxSeconds": 600
    }
  ],

leonardehrenfried · 2022-11-03T08:56:22Z

What if we create a new endpoint and enable it when APIGraphInspectorTile=true?

If you want it to part of the regular vector tiles endpoint, you can add a mapping in router-config.json.

hannesj · 2022-11-22T08:23:35Z

I will split the vector tiles API in tow, one as is in sandbox, and one in the core OTP code, which has a fixed set of debug layers.

t2gran

I a not qualified to review the JS code, but the rest looks ok.

t2gran · 2022-11-23T10:02:16Z

+/* This program is free software: you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License
+   as published by the Free Software Foundation, either version 3 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/


Should we remove this from all js files?

I think that is safe according to our LICENCE.md file.

We ac do that as a separate PR.

Required for compatibility with jts version 1.17

codecov-commenter · 2022-11-30T13:38:14Z

Codecov Report

Base: 60.70% // Head: 60.69% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (91d7f87) compared to base (eb8cf1b).
Patch coverage: 14.04% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #4565      +/-   ##
=============================================
- Coverage      60.70%   60.69%   -0.02%     
- Complexity     12240    12242       +2     
=============================================
  Files           1579     1584       +5     
  Lines          63414    63450      +36     
  Branches        6990     6987       -3     
=============================================
+ Hits           38498    38508      +10     
- Misses         22740    22768      +28     
+ Partials        2176     2174       -2

Impacted Files	Coverage Δ
...ipplanner/ext/vectortiles/VectorTilesResource.java	`21.05% <0.00%> (+12.45%)`	⬆️
...ers/stations/DigitransitStationPropertyMapper.java	`0.00% <ø> (ø)`
...tortiles/layers/stations/StationsLayerBuilder.java	`0.00% <0.00%> (ø)`
...es/layers/stops/DigitransitStopPropertyMapper.java	`87.09% <ø> (ø)`
...xt/vectortiles/layers/stops/StopsLayerBuilder.java	`0.00% <0.00%> (ø)`
.../DigitransitVehicleParkingGroupPropertyMapper.java	`100.00% <ø> (ø)`
...kings/DigitransitVehicleParkingPropertyMapper.java	`100.00% <ø> (ø)`
...arkings/StadtnaviVehicleParkingPropertyMapper.java	`81.25% <ø> (ø)`
...ayers/vehiclerental/VehicleRentalLayerBuilder.java	`0.00% <0.00%> (ø)`
...vehiclerental/VehicleRentalPlacesLayerBuilder.java	`0.00% <ø> (ø)`
... and 43 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

leonardehrenfried · 2022-11-30T16:22:52Z

        </dependency>
        <dependency>
-            <groupId>com.wdtinc</groupId>
+            <groupId>io.github.ci-cmg</groupId>


I took a look at this fork. If I'm not mistaken, the only thing it adds is the migration to a newer JTS version. Am I seeing this correctly?

wdtinc/mapbox-vector-tile-java@master...CI-CMG:mapbox-vector-tile:master

Yes, that is correct. We use a newer version of JTS, and the previous one crashed on serializing polylines.

leonardehrenfried · 2022-12-01T08:47:07Z

I tried it out with IBI's flex feeds in Atlanta and Seattle and it works really well.

leonardehrenfried · 2022-12-01T08:48:13Z

Thanks!

t2gran

In this review I was "very" picky, I normally try not to - sorry for that. Things like using the least open access modifier should not take extra time when writing code - the default should be private.

t2gran · 2022-12-04T16:27:40Z

+import org.locationtech.jts.geom.Envelope;

 /* http://wiki.openstreetmap.org/wiki/Slippy_map_tilenames */
 public class WebMercatorTile {


In this class there is a mixture of (y, x) and (lat, lon). I would prefer to stick to (lat, lon). Then in the JavaDoc one the class level we could mention that lat == x and lon == y in JTS. I hope I got it right ;-)

A JavaDoc comments would be nice. There are also some magic numbers in here witch to me is not obvious what means like tile2lat(y + 1, zoom), 180 and 360.

Most of the method are are public, but not used outside class/package -> convert them to private/package.

X and Y in this class refer to the x and y index of the map, used in the URL. I added some clarifications to the code and removed unused parts, and made helpers private

t2gran · 2022-12-04T16:49:55Z

+    double minLon = tile2lon(x, zoom);
+    double maxLon = tile2lon(x + 1, zoom);
+    return new Envelope(maxLon, minLon, maxLat, minLat);
+  }


The first 4 lines of the 2 methods above are duplicates - not sure if there is an easy way to avoid it.

I converted everything to the JTS envelope.

t2gran · 2022-12-04T16:54:28Z

+
+public class AreaStopsLayerBuilder extends LayerBuilder<AreaStop> {
+
+  private static final Map<MapperType, BiFunction<TransitService, Locale, PropertyMapper<AreaStop>>> mappers = Map.of(


If you give this a name BiFunction<TransitService, Locale, PropertyMapper<AreaStop>> the code become much easier to read. What I mean is to create a private functional interface at the bottom of this class.

t2gran · 2022-12-04T17:17:10Z

+      new T2<>("id", input.getId().toString()),
+      new T2<>("name", i18NStringMapper.mapNonnullToApi(input.getName()))


The T2 class is deleted, you need to make a record here.

This is pending on #4648

t2gran · 2022-12-04T17:18:41Z

+import org.opentripplanner.transit.model.site.AreaStop;
+import org.opentripplanner.transit.service.TransitService;
+
+public class DebugClientAreaStopPropertyMapper extends PropertyMapper<AreaStop> {


All public class definitions and methods should have JavaDoc, except methods inheriting the JavaDoc.

t2gran · 2022-12-04T17:29:00Z

The unit test coverage is going down int this PR.

…into otp2_debug_area_stops

leonardehrenfried · 2022-12-05T09:20:37Z

The unit test coverage is going down int this PR.

I'm not sure how it works exactly, but if you merge dev-2.x you get more up-do-date coverage data.

t2gran · 2022-12-05T12:24:57Z

+/**
+ * Common functionality for creating a vector tile response.
+ */
 public class VectorTileUtils {


Is VectorTileResponseFactory a better name?

hannesj added !New Feature A functional feature targeting the end user. +Sandbox This will be implemented as a Sandbox feature labels Nov 3, 2022

hannesj added this to the 2.3 milestone Nov 3, 2022

hannesj requested a review from a team as a code owner November 3, 2022 08:51

t2gran requested changes Nov 23, 2022

View reviewed changes

hannesj added 16 commits November 29, 2022 21:51

Update mapbox-vector-tile

c3caf6a

Required for compatibility with jts version 1.17

Expose findAreaStops

ece93f3

Add AreaStops vector tile layer

2c59314

Add AreaStops debug layer in debug client

f3213aa

refactor: move rsater tiles to separate package

ca15b84

Move mappers to inspector.vector

ea99f20

Use existing I18NStringMapper

acedd44

Move PropertyMapper to API

b60534e

Make interface internal

7efcc4a

Extract method

237875b

Extract TileJSON

39abda3

MoveLayerBuilder

ca43086

Extract layer mapping to function

4e4d21c

Clean up layer building

250f579

Extract LayersParameters

bf180b3

Add GraphInspectorVectorTileResource

38eb276

hannesj force-pushed the otp2_debug_area_stops branch from 61db73a to 38eb276 Compare November 30, 2022 13:27

Apply suggestion

d6d3332

hannesj requested review from leonardehrenfried and t2gran November 30, 2022 13:29

leonardehrenfried reviewed Nov 30, 2022

View reviewed changes

Comment thread src/main/java/org/opentripplanner/framework/io/HttpUtils.java Outdated

t2gran requested changes Dec 4, 2022

View reviewed changes

Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …

767bd73

…into otp2_debug_area_stops

hannesj added 4 commits December 5, 2022 11:33

Remove unused code from WebMercatorTile, convert to jts Envelope

0270dcb

Add name for mapper factory

48d7d46

Swap parameter order

4db5f7a

Update Javadoc

0ca0b7f

hannesj dismissed leonardehrenfried’s stale review via 0ca0b7f December 5, 2022 10:59

hannesj added 3 commits December 5, 2022 13:44

Add mapper test

e245b7a

Merge remote-tracking branch 'origin/dev-2.x' into otp2_debug_area_stops

575b83a

Make methods private

c4f805f

hannesj requested review from leonardehrenfried and t2gran December 5, 2022 11:58

leonardehrenfried previously approved these changes Dec 5, 2022

View reviewed changes

t2gran previously approved these changes Dec 5, 2022

View reviewed changes

t2gran reviewed Dec 5, 2022

View reviewed changes

Rename class

91d7f87

hannesj dismissed stale reviews from t2gran and leonardehrenfried via 91d7f87 December 5, 2022 12:46

hannesj requested review from leonardehrenfried and t2gran December 5, 2022 12:46

t2gran approved these changes Dec 5, 2022

View reviewed changes

leonardehrenfried approved these changes Dec 5, 2022

View reviewed changes

hannesj merged commit b2402c9 into opentripplanner:dev-2.x Dec 5, 2022

hannesj deleted the otp2_debug_area_stops branch December 5, 2022 13:04

t2gran pushed a commit that referenced this pull request Dec 5, 2022

Add changelog entry for #4565 [ci skip]

ea6330d

leonardehrenfried mentioned this pull request Dec 5, 2022

Upstream merge 2022-12-05 ibi-group/OpenTripPlanner#144

Merged


		public class AreaStopsLayerBuilder extends LayerBuilder<AreaStop> {

		private static final Map<MapperType, BiFunction<TransitService, Locale, PropertyMapper<AreaStop>>> mappers = Map.of(

		new T2<>("id", input.getId().toString()),
		new T2<>("name", i18NStringMapper.mapNonnullToApi(input.getName()))

Uh oh!

Conversation

hannesj commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

leonardehrenfried commented Nov 3, 2022

Uh oh!

hannesj commented Nov 22, 2022

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leonardehrenfried commented Dec 1, 2022

Uh oh!

leonardehrenfried commented Dec 1, 2022

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannesj Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t2gran commented Dec 4, 2022

Uh oh!

leonardehrenfried commented Dec 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hannesj commented Nov 3, 2022 •

edited

Loading

codecov-commenter commented Nov 30, 2022 •

edited

Loading

hannesj Dec 5, 2022 •

edited

Loading