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

Same road mode #46

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Same road mode #46

wants to merge 7 commits into from

Conversation

knvi
Copy link

@knvi knvi commented Dec 17, 2023

Implements #44

Performance-wise, I don't really see a difference. All tested.

@knvi
Copy link
Author

knvi commented Dec 17, 2023

Also closes #45

Copy link
Owner

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

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

(Sorry I've been slow to review; I'm on holiday for a while.)

Hmm, there's a logic bug. Even when the preview doesn't show a route continuing on a differently named street, I can click and create a disconnected point:

screencast.mp4

route-snapper/lib.js Outdated Show resolved Hide resolved
route-snapper/lib.js Outdated Show resolved Hide resolved
route-snapper/src/lib.rs Outdated Show resolved Hide resolved
@knvi
Copy link
Author

knvi commented Jan 2, 2024

My bad, i'll try to fix the problems asap

@knvi
Copy link
Author

knvi commented Jan 2, 2024

bez.tytuluf8.mp4

this should work now

@knvi knvi requested a review from dabreegster January 2, 2024 17:18
Copy link
Owner

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

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

Apologies again for the slow review; I have too many things going on at work to keep up. I only partly reviewed and found a few confusing things. I'll pick back up when I can make more time.

@@ -174,6 +174,8 @@
});
let idCounter = 0;
let snapTool = document.getElementById("snap-tool");
let lastRouteName = null;
Copy link
Owner

Choose a reason for hiding this comment

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

Unused

@@ -384,6 +402,9 @@ export class RouteSnapper {
#redraw() {
if (this.loaded) {
let gj = JSON.parse(this.inner.renderGeojson());
if (gj.length == 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

Fairly sure this is a bug. We might have been drawing something previously, then the user makes a change like deleting a waypoint, and there's no longer anything valid to draw. In that case, we still want to update MapLibre and stop drawing things.

};

if !is_connected(node1, current, &self.router) {
return String::from("[]");
Copy link
Owner

Choose a reason for hiding this comment

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

In this case maybe we don't want to draw the current extension to the route (this big if block), but we still want to draw any confirmed waypoints and parts of the route

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants