Skip to content

Show snappable nodes#3

Draft
dabreegster wants to merge 2 commits intomainfrom
show_snappables
Draft

Show snappable nodes#3
dabreegster wants to merge 2 commits intomainfrom
show_snappables

Conversation

@dabreegster
Copy link
Copy Markdown
Collaborator

Demo: https://acteng.github.io/atip/show_snappables/scheme.html?authority=TA_North+Yorkshire

Screencast from 11-09-24 15:59:54.webm
Some users have been confused when trying to snap to long roads without any snappable nodes. Also when changing from a freehand route to a snapped route, it's not always clear the user needs to pick a snapped node to resume. This PR styles all snappable nodes to show this more clearly. It's controllable manually, and automatically enabled when switching from freehand to snapped.

Also, the "avoid doubling back" option is removed from the controls. It's never really useful and only confusing.

Questions / concerns:

  • If a user doesn't like this behavior, is turning it on automatically for every free->snap transition annoying?
  • The nodes are drawn for zoom level 13 and above. At zoom 13, the nodes can be very clustered and messy. Should we only show for 14 or above maybe?
  • In larger areas, the first time these nodes are drawn, there's a bit of noticeable lag. With great effort we could move to web workers and make this async, but in the meantime, does it seem OK?

@JadeneAd
Copy link
Copy Markdown

Some thoughts:

  • Yes, if a user doesn't like this behaviour, turning it on automatically for every free->snap transition could be annoying. I think less so if it appears till the user has plotted their first two point and is clear they are back in the snap to roads mode. Then it switches off.

  • Yes lets try show nodes from zoom level 14 or above

  • It seems okay to me but happy to take Pete’s steer on this

@Pete-Y-CS
Copy link
Copy Markdown
Collaborator

I think that the snappable points staying visible after your first return to snapped pathfinding seems weird to me, which seems to be the behaviour: I would expect them to show, by default, only on unsnapped -> snapped transitions. Also I probably just have too many tabs open but when they first render my app was freezing up for a decent chunk of time (multiple seconds) surprisingly.

}
if (snapped) {
$showAllNodes = true;
// TODO Not sure why, but this isn't triggering the reactive statement above
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is it to do with it being a pointer effectively? is the reactive statement listening to the store itself rather than what's stored inside of it?

Copy link
Copy Markdown
Collaborator

@Pete-Y-CS Pete-Y-CS left a comment

Choose a reason for hiding this comment

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

I'm not sure what behaviour we want but the code looks fine to me


// Show snappable nodes when going from freehand to snapped
let first = true;
function freeToSnapped(snapped: boolean) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My expectation of this would work is that we would somehow listen to when the first snapped node is placed, when that happens switch showAllNodes off.

Also I would expect when snapped is false here we'd set $showAllNodes to false.

Assuming this is desired behaviour, though, the code looks fine.

@dabreegster
Copy link
Copy Markdown
Collaborator Author

when they first render my app was freezing up for a decent chunk of time (multiple seconds) surprisingly.

It's not just your computer, this will be expected in larger areas. There's not an easy way to speed things up beyond the current point. If we switched to web workers (multi-threading), we could make this async, so the tool wouldn't freeze up, but it would still take a few seconds for the nodes to appear the first time. That switch is complicated; if we decide it's necessary, I would make that switch separately from this PR. (And it'll have other UX implications, because every action the user takes will become decoupled from the effect by a slight time lag.)

I would expect them to show, by default, only on unsnapped -> snapped transitions.
Yes, if a user doesn't like this behaviour, turning it on automatically for every free->snap transition could be annoying. I think less so if it appears till the user has plotted their first two point and is clear they are back in the snap to roads mode. Then it switches off.


OK, it's sounding like what we want is:

  • the nodes aren't shown by default
  • when the user changes from unsnapped -> snapped, turn them on
  • if the user immediately switches back to unsnapped or after the user adds the first snapped node, hide them again
  • don't give the user a way to opt out of this behavior
  • Don't let the user turn on the snapping independently; only have this automatic case. Remove the new checkbox, in other words

Does that sound good?

@Pete-Y-CS
Copy link
Copy Markdown
Collaborator

when they first render my app was freezing up for a decent chunk of time (multiple seconds) surprisingly.

It's not just your computer, this will be expected in larger areas. There's not an easy way to speed things up beyond the current point. If we switched to web workers (multi-threading), we could make this async, so the tool wouldn't freeze up, but it would still take a few seconds for the nodes to appear the first time. That switch is complicated; if we decide it's necessary, I would make that switch separately from this PR. (And it'll have other UX implications, because every action the user takes will become decoupled from the effect by a slight time lag.)

I would expect them to show, by default, only on unsnapped -> snapped transitions.
Yes, if a user doesn't like this behaviour, turning it on automatically for every free->snap transition could be annoying. I think less so if it appears till the user has plotted their first two point and is clear they are back in the snap to roads mode. Then it switches off.

OK, it's sounding like what we want is:

  • the nodes aren't shown by default
  • when the user changes from unsnapped -> snapped, turn them on
  • if the user immediately switches back to unsnapped or after the user adds the first snapped node, hide them again
  • don't give the user a way to opt out of this behavior
  • Don't let the user turn on the snapping independently; only have this automatic case. Remove the new checkbox, in other words

Does that sound good?

This sounds like the best way to me

@dabreegster dabreegster marked this pull request as draft November 20, 2024 17:43
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.

3 participants