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

Geoscribble #1373

Merged
merged 2 commits into from
May 1, 2024
Merged

Geoscribble #1373

merged 2 commits into from
May 1, 2024

Conversation

Bonkles
Copy link
Contributor

@Bonkles Bonkles commented Apr 16, 2024

There are a number of things in play here! Built this after conferring with @bhousel and @tannerwuster on strategy. I copied some of the code from the existing custom data layer and trimmed out all the extras (file loading, mouse event handling, etc) and created a new endpoint to obtain the geojson for the scribbles.

  • A new Pixi layer to render the GeoScribbles.
  • A new service to obtain the geoscribbles.
  • Quick-and-dirty properties inspection in the sidebar.

Area shown at lng/lat 59.32609/24.78173:
https://github.com/facebook/Rapid/assets/1887955/5eaf632a-d544-4240-b4c4-e8b94dcece9d

image

@Bonkles Bonkles linked an issue Apr 16, 2024 that may be closed by this pull request
@Zverik
Copy link

Zverik commented Apr 17, 2024

Wow thank you Ben for taking this on! I have a few suggestions:

  1. Color is important for scribbles, because it shows the line type. Helps tell roads from barriers. Like, on the screenshot you provided, I only know what those lines mean because I drew them myself. With the given UI, one would need to hover over every single line to see what this means.
  2. Hence I would keep the color, but remove the hover info panel for scribbles. Because besides style (and maybe username and age in days), there is nothing useful there to mappers.
  3. I don't see how labels are rendered, and those would need such a panel to render the message.
  4. Ideally it would benefit from a "max age in days" slider, or a checkbox for "just the last month". But fine as it is.
  5. Looking at the code, maybe it would make sense to use the raw data endpoint /scribbles to query features? Granted, it's not geojson, but you don't need all properties to store.
  6. Also, there are no polygons in the data, guaranteed. Each feature has an identifier, and they cannot be updated, just created and deleted.
  7. After watching the video, I'd also suggest displaying this layer under OSM data, but on top of an imagery layer. The idea is that these scribbles mark what's missing in the data, so it might be hard to use them when they are on top.

@Bonkles
Copy link
Contributor Author

Bonkles commented Apr 17, 2024

Thanks for the great suggestions @Zverik - I've translated them into a series of to-dos in a comment on #1372.

I have a couple of questions to ask if you have the time:

  1. I don't see how labels are rendered, and those would need such a panel to render the message.

I'm not sure what you mean- can you explain more fully?

Also, I noticed there is a style member in the return. I presume that is from the draw_style code in EveryDoor. Should this further inform the styling somehow? It seems like it might be good information to impart to the user.

UPDATE: Ah, now I see in the example swagger response that there's a 'text' field that can be rendered too. So that would be something for us to potentially throw at the label system. @bhousel comic sans for the label font? lolol

@bhousel
Copy link
Contributor

bhousel commented Apr 17, 2024

Whoa cool - an editor property! I guess eventually Rapid users could also create scribbles and send them to the GeoScribble service?

@Bonkles
Copy link
Contributor Author

Bonkles commented Apr 17, 2024

Okay I've updated to include many of the suggestions. New video:

Screen.Recording.2024-04-17.at.12.27.15.PM.mov

@Zverik
Copy link

Zverik commented Apr 17, 2024

Wow, thanks for iterating so quickly, I did not expect that!

  • Currently I don't know whether the style attribute should be exposed to users. It may be useful, since we don't have a legend for the layer. But also I wouldn't want people to study each line individually. So that's up to you :)
  • Labels are a separate data type: those are points with attached messages. On the WMS layer (and in the app) I render them on the map, but in iD, it might make more sense to treat those as OSM notes: display an icon on top of the map (not under the data layer), and show the text on the sidebar. Again, up to you, but the main point is, those should be visible and the message should be readable. In the video, the "chain" note is hidden under a road.
  • Bryan, creating scribbles in Rapid is certainly possible :) But given this is an intermediate layer for passing surveyed data into editors, I think mapping the data directly is better than leaving scribbles, since we're already have the full-fledged editor open.

@bhousel bhousel added this to the v2.3 milestone Apr 17, 2024
@tordans
Copy link

tordans commented Apr 18, 2024

New video:

Looking at the video I wonder, that it will be very important to quickly toggle the layers via shortcuts. This will be important to better focus/see the scribble and then draw something in the editor. Do all those layers/actions already have shortcuts?


And speaking of those toggle-shortcuts, it might be a good idea to have a feature to show them (subtly) right in the UI (maybe flex+justify between on the right side of the box). I always struggle to remember the shortcuts and looking them up in the help menu is a big break in my mapping flow.


There are branch previews now in rapid, right? Maybe only fro non-draft PRs? I could not find the preview link…

@Bonkles
Copy link
Contributor Author

Bonkles commented Apr 18, 2024

Looking at the video I wonder, that it will be very important to quickly toggle the layers via shortcuts.

Completely agree- imagery switching, enabling/disabling different layers/modes etc. will be made much better with more shortcuts. Yet, there are only so many keys on the keyboard, so I think that to enable something like this, a dynamic keyboard rebinding system might be the way to go.

There are branch previews now in rapid, right? Maybe only fro non-draft PRs? I could not find the preview link…

There are, but because this branch has conflicts, the PR build-and-deploy doesn't fire. This is by design on github's part. I need to reconcile this branch with the massive changes that got landed yesterday, then the branch should deploy again.

Refactor the geojson extent calc out of custom data and into the util class, get the Geoscribbles populated in an Rbush.

Geoscribbles now appear on the map!

Remove polygon rendering code from the geoscribble layer.

Add thick/thin and color support for the scribbles.

Add dashed line support to the scribbles.

Only add dashes to lines that have dashed:true

Move the scribbles layer between the background and the map data, so it appears under OSM. Also remove interactivity for the layer so no more inspection panel popup.
Copy link
Contributor

@bhousel bhousel left a comment

Choose a reason for hiding this comment

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

This looks pretty good! Let's get it merged 🚀

@@ -20,6 +20,68 @@ export function utilTotalExtent(vals, graph) {
return extent;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call refactoring these into util/ - we are starting to use them everywhere.
Maybe we will move them into the rapid-sdk later 🤔

@bhousel bhousel merged commit 4264efe into main May 1, 2024
4 checks passed
@bhousel bhousel deleted the geoscribble branch May 1, 2024 15:34
@Zverik
Copy link

Zverik commented May 1, 2024

I've tested it juno now — lines work good, already made some improvements to the map based on my recent survey.

But I could not find how to read labels for point notes. Hovering over or clicking did not work, even with the OSM data layer turned off. Am I missing something?

@bhousel
Copy link
Contributor

bhousel commented May 1, 2024

But I could not find how to read labels for point notes. Hovering over or clicking did not work, even with the OSM data layer turned off. Am I missing something?

ah yeah I'm seeing this too - I'll figure out what's going on

bhousel added a commit that referenced this pull request May 1, 2024
(re: #1373)

- indent, documentation
- some simplification where we can use Extent's builtin functions to do things (.bbox, .rectangle, .extendSelf)
- remove a few unused things (inflightPost)
@bhousel
Copy link
Contributor

bhousel commented May 1, 2024

But I could not find how to read labels for point notes. Hovering over or clicking did not work, even with the OSM data layer turned off. Am I missing something?

For now , in 0c32a17 I restored the interactivity so that users can see the data associated with the scribbles, and we can render any text through the existing labeling code. We can improve it more later, but this seems ok for today.. It will be interesting to see how users use this!

Screenshot 2024-05-01 at 5 41 22 PM

@Zverik
Copy link

Zverik commented May 3, 2024

Thank you Bryan! Good enough! Added the editor to the wiki list :)

bhousel added a commit that referenced this pull request May 8, 2024
re: osmlab/editor-layer-index#2312

We built a service for this in #1373, so we don't want it to
also show up as a raster overlay
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

geoscribble notes support
5 participants