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

Make the route loading more obvious. #260

Merged
merged 9 commits into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
292 changes: 146 additions & 146 deletions package-lock.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we revert the changes here please? Guessing this was a rebase problem. Without changes to package.json, I wouldn't expect the lock file to change generally

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"vite-plugin-wasm-pack": "0.1.11"
},
"dependencies": {
"@megapenthes/svelte4-progressbar": "1.0.1",
"@turf/bbox": "^6.5.0",
"@turf/boolean-point-in-polygon": "^6.5.0",
"@turf/helpers": "^6.5.0",
Expand Down
52 changes: 52 additions & 0 deletions src/lib/FetchProgressBar.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<script lang="ts">
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks unused. Is the intention to keep this or the one in RouteMode? I'd vote for having the progress bar logic wrapped up in its own component, not in the middle of RouteMode. Especially since we may want to use this for route info (worker.ts) in some form in the future.

import { onMount } from "svelte";

export let url: string;
export let onSuccess: (bytes: Uint8Array) => void;

// Both are units of bytes
let bytesReceived = 0;
let maxBytes = 100;
let progressBar = {
style: "background: linear-gradient(to right, red 0%, transparent 0);",
};

onMount(async () => {
console.log(`Fetching ${url} with a progress bar`);
let response = await fetch(url);
let reader = response.body.getReader();

// TODO Handle when missing?
maxBytes = parseInt(response.headers.get("Content-Length"));

let chunks = [];
while (true) {
let { done, value } = await reader.read();
if (done) {
break;
}

chunks.push(value);
bytesReceived += value.length;

const percent = (bytesReceived / maxBytes) * 100;
progressBar.style = `background: linear-gradient(to right, red ${percent}%, transparent 0);`;
}

let outputBytes = new Uint8Array(maxBytes);
let position = 0;
console.log(
`max bytes ${maxBytes} - bytes received ${bytesReceived} - progress bar style ${progressBar.style}`
);
for (let chunk of chunks) {
console.log(
`position ${position}, outputBytes ${outputBytes.length} - chunk length ${chunk.length}`
);
outputBytes.set(chunk, position);
position += chunk.length;
}
onSuccess(outputBytes);
});
</script>

<progress style={progressBar.style} value={bytesReceived} max={maxBytes} />
60 changes: 52 additions & 8 deletions src/lib/draw/route/RouteMode.svelte
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<script lang="ts">
import ProgressBar from "@megapenthes/svelte4-progressbar";
import type { LineString } from "geojson";
import init from "route-snapper";
import { fetchWithProgress } from "route-snapper/lib.js";
import { onMount } from "svelte";
import type { FeatureWithProps } from "../../../maplibre_helpers";
import { currentMode, map } from "../../../stores";
import { currentMode, map, routeInfo } from "../../../stores";
import type { Mode } from "../../../types";
import { handleUnsavedFeature, setupEventListeners } from "../common";
import type { EventHandler } from "../event_handler";
Expand All @@ -16,10 +16,12 @@
export let changeMode: (m: Mode) => void;
export let url: string;

let progress: HTMLDivElement;
export let routeTool: RouteTool;
export let eventHandler: EventHandler;

let progress: Array<number> = [0 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an array with just one element?

(also, npm run fmt shows a diff here and elsewhere)

let routeToolReady = false;

// While the new feature is being drawn, remember its last valid version
let unsavedFeature: { value: FeatureWithProps<LineString> | null } = {
value: null,
Expand All @@ -45,11 +47,13 @@

console.log(`Grabbing ${url}`);
try {
const graphBytes = await fetchWithProgress(url, progress);
routeTool = new RouteTool($map, graphBytes);
const graphBytes = await fetchWithProgress(
url,
(percentLoaded) => (progress[0] = Math.min(percentLoaded, 90))
Copy link
Contributor

Choose a reason for hiding this comment

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

So here's where we say "we got all of Content-Length, cap at 90". This is subtle / unintuitive, so definitely worth a comment

);
routeTool = new RouteTool($map, graphBytes, routeInfoDeserialised);
} catch (err) {
console.log(`Route tool broke: ${err}`);
progress.innerHTML = "Failed to load";
return;
}

Expand All @@ -61,11 +65,51 @@
changeMode
);
});

async function fetchWithProgress(
url,
setProgress: (number) => void = (percentLoaded) => {}
) {
const response = await fetch(url);
const reader = response.body.getReader();

const contentLength = parseInt(response.headers.get("Content-Length"));

let receivedLength = 0;
let chunks = [];
while (true) {
const { done, value } = await reader.read();
if (done) {
break;
}

chunks.push(value);
receivedLength += value.length;

const percent = (100.0 * receivedLength) / contentLength;
setProgress(percent);
}

let allChunks = new Uint8Array(receivedLength);
let position = 0;
for (let chunk of chunks) {
allChunks.set(chunk, position);
position += chunk.length;
}

return allChunks;
}

function routeInfoDeserialised() {
progress[0] = 100;
routeToolReady = true;
}
</script>

{#if !routeTool}
{#if !routeToolReady}
<!-- TODO the text should be fixed, and the progress bar float -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment becomes out of date with this PR

<div bind:this={progress}>Route tool loading...</div>
<p>Route tool loading</p>
<ProgressBar bind:series={progress} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how this new dependency helps us. It can draw lots of fancier progress bars, but we just want a simple linear one. In the spirit of keeping dependencies low, shouldn't we avoid it? We could maybe replace the older CSS hacks with https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress

{:else if $currentMode == thisMode}
<RouteControls {routeTool} extendRoute />
{/if}
3 changes: 2 additions & 1 deletion src/lib/draw/route/route_tool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,12 @@ export class RouteTool {
) => void)[];
eventListenersFailure: (() => void)[];

constructor(map: Map, graphBytes: Uint8Array) {
constructor(map: Map, graphBytes: Uint8Array, deserialisedCallback=()=>{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This has only one caller, who sets the callback, so a default value seems odd. How about we just write the type? : () => void

this.map = map;
console.time("Deserialize and setup JsRouteSnapper");
this.inner = new JsRouteSnapper(graphBytes);
console.timeEnd("Deserialize and setup JsRouteSnapper");
deserialisedCallback();
this.active = false;
this.eventListenersSuccess = [];
this.eventListenersUpdated = [];
Expand Down
Loading