Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop Grouping with Distance Range and (General) Directional filter #193

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/components/bookmarked-stop/StopRouteList.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import { Box, CircularProgress, List, SxProps, Theme } from "@mui/material";
import SuccinctTimeReport from "../home/SuccinctTimeReport";
import { useStopGroup } from "../../hooks/useStopGroup";
import { useStopEtas } from "../../hooks/useStopEtas";
import { Company } from "hk-bus-eta";

interface StopRouteListProps {
stops: Array<[Company, string]>; // [[co, stopId]]
routeId : string | undefined;
isFocus: boolean;
}

const StopRouteList = ({ stops, isFocus }: StopRouteListProps) => {
const stopEtas = useStopEtas({ stopKeys: stops, disabled: !isFocus });
const StopRouteList = ({ stops, routeId = undefined, isFocus }: StopRouteListProps) => {
const stopGroup = useStopGroup({ stopKeys: stops, routeId : routeId });
const stopEtas = useStopEtas({ stopKeys: stopGroup, disabled: !isFocus });

if (stopEtas.length === 0) {
return (
Expand Down
1 change: 1 addition & 0 deletions src/components/bookmarked-stop/SwipeableStopList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ const SwipeableStopList = React.forwardRef<
<StopRouteList
key={`savedStops-${idx}`}
stops={stops}
routeId={undefined}
isFocus={getViewIdx() === idx}
/>
))}
Expand Down
5 changes: 3 additions & 2 deletions src/components/route-eta/StopDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@ import CollectionContext from "../../CollectionContext";

interface StopDialogProps {
open: boolean;
routeId : string;
stops: Array<[Company, string]>;
onClose: () => void;
}

const StopDialog = ({ open, stops, onClose }: StopDialogProps) => {
const StopDialog = ({ open, routeId, stops, onClose }: StopDialogProps) => {
const {
db: { stopList },
} = useContext(DbContext);
Expand Down Expand Up @@ -83,7 +84,7 @@ const StopDialog = ({ open, stops, onClose }: StopDialogProps) => {
</Box>
</DialogTitle>
<DialogContent>
<StopRouteList stops={stops} isFocus={true} />
<StopRouteList routeId={routeId} stops={stops} isFocus={true} />
</DialogContent>
</Dialog>
);
Expand Down
181 changes: 181 additions & 0 deletions src/hooks/useStopGroup.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
import { useContext, useMemo } from "react";
import { Company } from "hk-bus-eta";
import type { StopListEntry } from "hk-bus-eta";
import DbContext from "../context/DbContext";
import { getDistance, getBearing } from "../utils";

interface useStopGroupProps {
stopKeys: Array<[Company, string]>;
routeId : string | undefined;
}
interface StopListEntryExtended extends StopListEntry {
id : string;
distance : number;
routeStops : RouteStopCoSeq[];
}
interface RouteStopCoSeq {
routeKey : string;
co : Company;
seq : number;
}

// stopKey in format "<co>|<stopId>", e.g., "lightRail|LR140"
export const useStopGroup = ({
stopKeys, routeId
}: useStopGroupProps) => {
const {
db: { routeList, stopList },
} = useContext(DbContext);

// TODO: put these constants in AppContext user preference
const DISTANCE_THRESHOLD = 50; // in metres, will recursively find stops within this number of metres, so keep it as small as possible. Never choose larger than 300m.
const BEARING_THRESHOLD = 45; // in degrees (°), acceptable deviation to the left or right of current bearing
const STOP_LIST_LIMIT = 50; // max number of stops in a group, if more than that, the ETA list will be too long and meaningless

const getDistanceStop = (a : StopListEntry, b : StopListEntry) => {
return getDistance(a.location, b.location);
};
const getBearingStops = (a : StopListEntry, b : StopListEntry) => {
return getBearing(a.location, b.location);
};

const getAllRouteStopsBearings = (routeStops : RouteStopCoSeq[]) => {
// routeStop example: {"routeKey":"101+1+KENNEDY TOWN+KWUN TONG (YUE MAN SQUARE)","co":"ctb","seq":12}
return routeStops.map(routeStop => {
const { routeKey, co, seq } = routeStop;
const stopLength = routeList[routeKey].stops[co].length;
let stopA, stopB;
if(seq == stopLength - 1) { // last stop
// no next stop, hence no forward bearing, just use -1 as dummy value then discard it later
return -1;
} else {
stopA = stopList[routeList[routeKey].stops[co][seq]];
stopB = stopList[routeList[routeKey].stops[co][seq + 1]];
}
return getBearingStops(stopA, stopB);
}).filter(brng => brng !== -1);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize the getAllRouteStopsBearings Function.

The function definition is correct but can be optimized by removing the unnecessary else clause.

-  if(seq == stopLength - 1) { // last stop
-    // no next stop, hence no forward bearing, just use -1 as dummy value then discard it later
-    return -1;
-  } else {
-    stopA = stopList[routeList[routeKey].stops[co][seq]];
-    stopB = stopList[routeList[routeKey].stops[co][seq + 1]];
-  }
+  if(seq == stopLength - 1) return -1; // last stop
+  stopA = stopList[routeList[routeKey].stops[co][seq]];
+  stopB = stopList[routeList[routeKey].stops[co][seq + 1]];
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const getAllRouteStopsBearings = (routeStops : RouteStopCoSeq[]) => {
// routeStop example: {"routeKey":"101+1+KENNEDY TOWN+KWUN TONG (YUE MAN SQUARE)","co":"ctb","seq":12}
return routeStops.map(routeStop => {
const { routeKey, co, seq } = routeStop;
const stopLength = routeList[routeKey].stops[co].length;
let stopA, stopB;
if(seq == stopLength - 1) { // last stop
// no next stop, hence no forward bearing, just use -1 as dummy value then discard it later
return -1;
} else {
stopA = stopList[routeList[routeKey].stops[co][seq]];
stopB = stopList[routeList[routeKey].stops[co][seq + 1]];
}
return getBearingStops(stopA, stopB);
}).filter(brng => brng !== -1);
}
const getAllRouteStopsBearings = (routeStops : RouteStopCoSeq[]) => {
// routeStop example: {"routeKey":"101+1+KENNEDY TOWN+KWUN TONG (YUE MAN SQUARE)","co":"ctb","seq":12}
return routeStops.map(routeStop => {
const { routeKey, co, seq } = routeStop;
const stopLength = routeList[routeKey].stops[co].length;
let stopA, stopB;
if(seq == stopLength - 1) return -1; // last stop
stopA = stopList[routeList[routeKey].stops[co][seq]];
stopB = stopList[routeList[routeKey].stops[co][seq + 1]];
return getBearingStops(stopA, stopB);
}).filter(brng => brng !== -1);
}
Tools
Biome

[error] 51-54: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)


const routeStops : RouteStopCoSeq[]= [];
if(routeId !== undefined) {
// StopDialog
let targetRouteStops = routeList[routeId].stops;
stopKeys.forEach(([co, stopId]) => {
let seq = targetRouteStops[co]?.indexOf(stopId) ?? -1;
if(seq != -1) {
routeStops.push({
routeKey : routeId,
co : co,
seq : seq
});
}
});
} else {
// SwipableStopList (saved stop list)
stopKeys.forEach(([co, stopId]) => {
Object.keys(routeList).forEach(routeKey => {
let seq = routeList[routeKey]?.stops[co]?.indexOf(stopId) ?? -1;
if(seq != -1) {
routeStops.push({
routeKey : routeKey,
co : co,
seq : seq
});
}
})
});
}
const bearingTargets = getAllRouteStopsBearings(routeStops);
const isBearingInRange = (bearing : number) => {
if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) {
return true;
} else {
for(let i = 0; i < bearingTargets.length; ++i) {
let bearingMin = bearingTargets[i] - BEARING_THRESHOLD;
let bearingMax = bearingTargets[i] + BEARING_THRESHOLD;
if(bearingMin < 0)
bearingMin += 360;
if(bearingMax > 360)
bearingMax -= 360;
if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax)
|| (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020°
return true;
}
return false;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize the isBearingInRange Function.

The function definition is correct but can be optimized by removing the unnecessary else clause.

-  if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) {
-    return true;
-  } else {
-    for(let i = 0; i < bearingTargets.length; ++i) {
-      let bearingMin = bearingTargets[i] - BEARING_THRESHOLD;
-      let bearingMax = bearingTargets[i] + BEARING_THRESHOLD;
-      if(bearingMin < 0)
-        bearingMin += 360;
-      if(bearingMax > 360)
-        bearingMax -= 360;
-      if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax)
-      || (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020°
-        return true;
-    }
-    return false;
-  }
+  if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) return true;
+  for(let i = 0; i < bearingTargets.length; ++i) {
+    let bearingMin = bearingTargets[i] - BEARING_THRESHOLD;
+    let bearingMax = bearingTargets[i] + BEARING_THRESHOLD;
+    if(bearingMin < 0) bearingMin += 360;
+    if(bearingMax > 360) bearingMax -= 360;
+    if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax)
+    || (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020°
+      return true;
+  }
+  return false;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const bearingTargets = getAllRouteStopsBearings(routeStops);
const isBearingInRange = (bearing : number) => {
if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) {
return true;
} else {
for(let i = 0; i < bearingTargets.length; ++i) {
let bearingMin = bearingTargets[i] - BEARING_THRESHOLD;
let bearingMax = bearingTargets[i] + BEARING_THRESHOLD;
if(bearingMin < 0)
bearingMin += 360;
if(bearingMax > 360)
bearingMax -= 360;
if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax)
|| (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020°
return true;
}
return false;
}
}
const bearingTargets = getAllRouteStopsBearings(routeStops);
const isBearingInRange = (bearing : number) => {
if(BEARING_THRESHOLD >= 180 || bearingTargets.length == 0) return true;
for(let i = 0; i < bearingTargets.length; ++i) {
let bearingMin = bearingTargets[i] - BEARING_THRESHOLD;
let bearingMax = bearingTargets[i] + BEARING_THRESHOLD;
if(bearingMin < 0) bearingMin += 360;
if(bearingMax > 360) bearingMax -= 360;
if((bearingMin <= bearingMax && bearingMin <= bearing && bearing <= bearingMax)
|| (bearingMin > bearingMax && (bearingMin <= bearing || bearing <= bearingMax))) // crossing 0/360° mark, eg min=340°,max=020°
return true;
}
return false;
}
Tools
Biome

[error] 92-106: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)


const stopGroup = useMemo<Array<[Company, string]>>(() => {
let stopListEntries : StopListEntryExtended[] = [];
const searchNearbyStops = (targetStopId : string, excludedStopIdList : string[]) => {
const targetStop = stopList[targetStopId];

return Object.keys(stopList).filter((stopId) => {
// find stops that are within DISTANCE_THRESHOLD metres of target stop and along similar direction
return getDistanceStop(targetStop, stopList[stopId]) <= DISTANCE_THRESHOLD &&
// filter out stops that have not been added into excludeList before
!excludedStopIdList.includes(stopId);
})
.reduce( (acc, stopId) => {
// get all the routes that has stop with this stopId
const _routeStops = Object.entries(routeList).map(([routeKey, routeListEntry]) => {
const stops = routeListEntry.stops ?? {};
const companies = Object.keys(stops) as Company[];
for(let co of companies) {
let stopPos = stops[co]?.indexOf(stopId) ?? -1;
if(stopPos > -1)
return { routeKey : routeKey, co : co, seq : stopPos } as RouteStopCoSeq;
}
return { routeKey : routeKey, co : 'ctb', seq : -1 } as RouteStopCoSeq; // use ctb as dummy value and seq = -1, will be discarded in next filter
})
.filter((_rs) => _rs.seq != -1);
// if any of the routes passing this stop is facing same direction (+/- BEARING_THRESHOLD), add the stop to the list
// Note: once the stop is added, other routes not facing same direction but passing this stop will also be shown in ETA (most commonly seen in railway lines)
const bearings = getAllRouteStopsBearings(_routeStops);
if(bearings.find(b => isBearingInRange(b)) !== undefined) {
const thisStop : StopListEntryExtended = {
...stopList[stopId],
id : stopId,
routeStops : _routeStops, // _routeStops.length must be > 0 here, as bearings.length must be > 0 to reach into this if-condition
distance : 0 // dummy value
};
acc.push(thisStop);
}
return acc
}, [] as Array<StopListEntryExtended>);
}

// recursively search for nearby stops within thresholds (distance and bearing)
// stop searching when no new stops are found within range, or when stop list is getting too large
stopKeys.forEach((stopKey) => {
const [, stopId] = stopKey; // [co, stopId]
stopListEntries = stopListEntries.concat(searchNearbyStops(stopId, stopListEntries.map((stop) => stop.id)));
for(let i = 0; i < stopListEntries.length; ++i) { // use traditional for-loop as the length keeps expanding
stopListEntries = stopListEntries.concat(searchNearbyStops(stopListEntries[i].id, stopListEntries.map((stop) => stop.id)));
if(stopListEntries.length >= STOP_LIST_LIMIT)
break;
}
});

// sort by distance from first stop in stopMap (stopKeys[0])
const _stopGroup : Array<[Company, string]> = [];
if(stopKeys.length > 0) {
let [, stopId] = stopKeys[0]; // [co, stopId] but don't use this co
stopListEntries = stopListEntries.map(stop => {
return {
... stop,
distance : getDistanceStop(stopList[stopId], stop)
};
}).sort((stopA, stopB) => {
return stopA.distance - stopB.distance
});
stopListEntries.forEach((stopListEntry) => {
if(stopListEntry.routeStops.length > 0)
_stopGroup.push([stopListEntry.routeStops[0].co, stopListEntry.id]);
});
}
return _stopGroup;
}, [stopKeys, routeList, stopList]);

Check warning on line 178 in src/hooks/useStopGroup.tsx

View workflow job for this annotation

GitHub Actions / build (20.x)

React Hook useMemo has missing dependencies: 'getAllRouteStopsBearings' and 'isBearingInRange'. Either include them or remove the dependency array

Check warning on line 178 in src/hooks/useStopGroup.tsx

View workflow job for this annotation

GitHub Actions / build (21.x)

React Hook useMemo has missing dependencies: 'getAllRouteStopsBearings' and 'isBearingInRange'. Either include them or remove the dependency array

return stopGroup;
};
1 change: 1 addition & 0 deletions src/pages/RouteEta.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ const RouteEta = () => {
/>
<StopDialog
open={isDialogOpen}
routeId={routeId}
stops={dialogStop}
onClose={handleCloseDialog}
/>
Expand Down
13 changes: 13 additions & 0 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,19 @@ export const getDistanceWithUnit = (distanceInMetre: number) => {
};
};

export const getBearing = (a: GeoLocation, b: GeoLocation) => {
// Reference: https://www.movable-type.co.uk/scripts/latlong.html
const φ1 = a.lat * Math.PI / 180; // φ, λ = lat, lon in radians
const φ2 = b.lat * Math.PI / 180;
const λ1 = a.lng * Math.PI / 180; // φ, λ = lat, lon in radians
const λ2 = b.lng * Math.PI / 180;
const y = Math.sin(λ2 - λ1) * Math.cos(φ2);
const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(λ2-λ1);
const θ = Math.atan2(y, x);
const brng = (θ * 180 / Math.PI + 360) % 360; // in degrees
return brng;
};
Comment on lines +37 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize variable declarations and add comments for clarity.

The function is correctly implemented but can be optimized and improved for readability by combining the variable declarations and adding comments to explain the steps.

export const getBearing = (a: GeoLocation, b: GeoLocation) => {
  // Reference: https://www.movable-type.co.uk/scripts/latlong.html
- const φ1 = a.lat * Math.PI / 180; // φ, λ = lat, lon in radians
- const φ2 = b.lat * Math.PI / 180;
- const λ1 = a.lng * Math.PI / 180; // φ, λ = lat, lon in radians
- const λ2 = b.lng * Math.PI / 180;
- const y = Math.sin(λ2 - λ1) * Math.cos(φ2);
- const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(λ2-λ1);
- const θ = Math.atan2(y, x);
- const brng = (θ * 180 / Math.PI + 360) % 360; // in degrees
- return brng;
+ const φ1 = a.lat * Math.PI / 180, φ2 = b.lat * Math.PI / 180;
+ const λ1 = a.lng * Math.PI / 180, λ2 = b.lng * Math.PI / 180;
+ 
+ // Calculate the differences
+ const Δλ = λ2 - λ1;
+ const y = Math.sin(Δλ) * Math.cos(φ2);
+ const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(Δλ);
+ 
+ // Calculate the bearing
+ const θ = Math.atan2(y, x);
+ return (θ * 180 / Math.PI + 360) % 360; // in degrees
};
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const getBearing = (a: GeoLocation, b: GeoLocation) => {
// Reference: https://www.movable-type.co.uk/scripts/latlong.html
const φ1 = a.lat * Math.PI / 180; // φ, λ = lat, lon in radians
const φ2 = b.lat * Math.PI / 180;
const λ1 = a.lng * Math.PI / 180; // φ, λ = lat, lon in radians
const λ2 = b.lng * Math.PI / 180;
const y = Math.sin(λ2 - λ1) * Math.cos(φ2);
const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(λ2-λ1);
const θ = Math.atan2(y, x);
const brng = (θ * 180 / Math.PI + 360) % 360; // in degrees
return brng;
};
export const getBearing = (a: GeoLocation, b: GeoLocation) => {
// Reference: https://www.movable-type.co.uk/scripts/latlong.html
const φ1 = a.lat * Math.PI / 180, φ2 = b.lat * Math.PI / 180;
const λ1 = a.lng * Math.PI / 180, λ2 = b.lng * Math.PI / 180;
// Calculate the differences
const Δλ = λ2 - λ1;
const y = Math.sin(Δλ) * Math.cos(φ2);
const x = Math.cos(φ1) * Math.sin(φ2) - Math.sin(φ1) * Math.cos(φ2) * Math.cos(Δλ);
// Calculate the bearing
const θ = Math.atan2(y, x);
return (θ * 180 / Math.PI + 360) % 360; // in degrees
};


export const DEFAULT_GEOLOCATION: GeoLocation = {
lat: 22.302711,
lng: 114.177216,
Expand Down
Loading