Streetview Imagery bug bash #1526
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The main change here is in AbstractLayer and the various Photo PixiLayerXX. What should happen is that these pseudo classes are applied when rendering. As a layer processes its features, it calls `syncFeatureClasses` to determine if any of the features needs some special styling. This function will dirty the feature if any of these classes had changed. But 'active'/'drawing' was mixed up. I think what might have happened before was: Drag Behavior set `feature.active = true` to avoid emitting events But it didn't set this 'active' class on the layer (to make it stick) Then eventually `syncFeatureClasses` would run and un-set that 'active' class This probably messed up the dragging so somebody changed `syncFeatureClasses` to treat `active` differently. But then this made the streetside markers weird, so somebody added extra code to the photo layers to look for which photo was 'active' This commit tries to sort all of that out, and also update some docs and variable naming.
Sometimes we need to wait for a texture to be ready, so we need to leave `_styleDirty=true` in that situation. The previous code would actually leave `_styleDirty=true` even in situations where the was no icon or marker, the new code better checks if we are really waiting or not. This had the surprising side effect of, always leaving viewfield markers dirty (like the mapillary view cones) beccause they have no icon. I could not figure out why the view cone could still rerender with updated angles or get restyled without being specificaly set to be dirty - it was because of this unintended side effect. This should also make viewcone rendering a little more efficient.
The pannellum viewer has more than 2 seconds of inertia when you let go and let it spin.
The viewer is still open and showing the photo. We should unselect the photo only when the viewer closes.
The unclass code was only considering the current layer, but it should really unclass all photo layers. This was causing for example, if you switch between a streetside photo to a mapillary photo, it would leave the selection halo set on both markers. This also unrelated, updates the documentation for the `photosUsed` and `imageryUsed` functions.
Mapillary viewer has a fallback mode for situations where a WebGL environment is not available. Now that Rapid requires WebGL to do everything, we can remove anything related to fallback mode.
Using the convention where: $thing - a d3 selection $$thing - a d3 enter selection
(closes #1509) There's a bunch of cleanup in here. The main issues with KartaView were: - The `_maxPageAtZoom` function was being passed float zooms not integer zooms, so it was failing and the code was attempting to fetch page `Infinity` - The old `https://api.openstreetcam.org/2.0/photo/` API was being called with a `sequenceId=` param to fetch all the urls for all photos on sequence. This was unnecessary (we only need the URL we are about to show) so I changed it to just get a single photo. (doing it the other way was actually returning paginated results, so we'd only ever show the first 99 photos in a sequence) - `selectImageAsync` and promises means that we can eliminate the `waitingForImage` variable that wasn't actually being used correctly anyway. - I fixed active styling the KartaView images too. - Also added `isPano` in a few places to prepare for 360deg Kartaview images. - probably other things
Previously this was only working for Mapillary photo layer, but we should style our markers consistently across all layers. This also removes some unnecessary code from the MapillaryService to keep track of the current active image (PhotoSystem does this now)
This has bothered me for a while, we should not class something as hover if it is already drawing.
The previous few commits were leading up to this. We use these pseudo classes for a lot of things, both styling but also some behaviors. They weren't documented very well. Applying the 'active' class to decide whether a photo was selected was causing all kinds of problems, becasue that class also makes the feature non-interactive. This had the effect of, sometimes you could switch from one photo marker to another, it would leave the first one styled as selected, but also make it unclickable. Just making a new pseudo class for photo styling means this isn't a problem now, and it's a lot easier to keep track of which photo is the selected one. Note that the 'selected' class also applies, it's the thing that draws the dashed line halo around it. Another feature can get 'selected', for example if the user clicks on some OSM feature, but as long as the photo is still showing, we stil want to keep the 'photoselected' class applied.
We had a bunch of places where Mapillary viewer and Pannellum viewer were really different.. This makes them a bit more similar. - Both services dispatch the same events now - Mapillary viewer's getZoom/getBearing/getFov are promisified for some reason. So we wait until that promise has settled before dispatching the fovChanged event. - Using fov (field of view) everywhere now, instead of Mapillary Viewer's made up 0-3 zoom values that correspond to +/- zoom buttons - Trying to avoid holding onto state in the PixiLayerWhateverPhotos classes (but still need to in Mapillary's case, because there is no sync getFov/getBearing) - The mapillary view cone will sometimes flash a different direction and then swing into the correct direction, and this is a bit weird, but it matches with the viewer is doing when it switches photos.
(closes #1511) We are getting these from a vector tile service, so they will arrive to us as a bunch of LineString or MultiLineStrings.. We can treat them like a FeatureCollection, but we have to handle all the sub-parts we might get.
(closes #1512) This also contains a few other random filtering fixes - Simplified the `filterImages`/`filterSequences` functions - Streetside sequences should be set as `isPano=true` (so that if a user unchecks "panoramic photos" the filter works) - KartaView sequences use sparse arrays, so there was code in there that was doing `image[0]?.captured_at`, `image[0]?.captured_by` that would generally fail. Now we .find any image actually in the array to get these properties.
(closes #1513) The root cause of this was checking `lastv` in the `loadTiles()` function. This is something I added in v2.3 to avoid doing a lot of work to load data if the viewport hasn't moved since the last `loadTiles()` call. In the case of Mapillary, we actually may load up to 3 things: photos, detections, and signs so each of these caches needs their own lastv, and can't use a shared lastv This also includes other changes to make things a bit nicer: - Rename MapillaryFeatures -> MapillaryDetections (because features can mean anything) We will call these things "detections" more consistently (not "objects", "features", "points") - Clean up all the streetlevel translation strings, they were a mess - Move the detections and signs markers up to the "qa" render group, so they are drawn above the streetlevel viewcones and sequences (this looks nicer and makes them easier to select) - Fix the tests - Lots of variable renames and jsdoc comments
The text is actually keyed like "example-normal" or "example-italic" so we need to use the correct textureID in both `getTexture` and `allocate` This was likely causing the same text to get repeatedly inserted into the atlas a lot of times, probably causing performance issues.
For recap, these classes are how we adjust the style of features now that we
can't use CSS classes to do it. They are strings that are owned by each layer.
We created a bunch of them like 'drawing', 'highlight', 'selected', etc.
and then at render time we call `syncFeatureClasses` to update each feature with
what kind of classes it should have.
Working on the streetlevel imagery I realized that I want more pseudo classes
to highlight image viewcones, and the list is getting long, so it's time to stop
hardcoding these special properties and handle them dynamically.
This also gives us an oppotrunity to clean up the ones that were confusing,
redundand, or not really used.
So here's what has changed:
- Removed all the AbstractFeature getter/setters and now have `setClass/unsetClass/hasClass`
- AbstractFeature now has a private `_classes` Set<classIDs>
- setClass/unsetClass now take care of dirtying the style and label
- `feature.highlighted = true` -> `feature.setClass('highlight)`
- `feature.drawing = false` -> `feature.unsetClass('drawing')`
- etc..
- Perfer shorter names for the classes, e.g. "select" instead of "selected"
- Some renames and adjustment of arglists in the PixiScene and AbstractLayer
- `classData(dataID, classID)` -> `setClass(classID, dataID)`
- `unclassData(dataID, classID)` -> `unsetClass(classID, dataID)
- etc.. the class name comes first, like `setClass('hover', 'w123')`
- Updated lots of documentation
- 'active' was used for interactivity, but it didn't really work, it's removed now
- this only really affects `DragBehavior`, when dragging things
- We also had `feature.allowInteraction` it does the same thing, we just use that now
This paves the way for me to do the thing I want to do with the photo viewcones
without adding more hardcoded classes.
(re: #1515) Work in Progress on: - Style improvement for detected map features - If a detection is selected, highlight the images that show that detection This works, but isn't great. Some problems are: - the detections aren't very precise - the icons don't look good - the images picked that show the detection often don't show it well (need to improve tag/segmentation styling) - We're showing a lot of features that maybe we shouldnt (road markings) which clutter the map - Race condition where multiple things are trying to centerEase the map Also should adjust the zoom to better show what we are trying to show. (a selected detection with several highlighted images roughly pointing at it)
Before, the viewfield textures were generated with 0.75 alpha. Now, they are generated with 1 alpha and can be overridden with a style property. This allows us to adjust the viewfield alpha as the user does things e.g, selects, hovers etc, (previously how things worked with the svg renderer) Also starting to remove the vestigial svg css rules still hanging around in 60_photos.css
What would happen before was that many places in the code were trying to centerEase the map to center on either the image or the detection, and this was jarring. now: - This is now PhotoSystem's job. Only `selectPhoto` or `selectDetection` will try to to centerEase the map. - It will happen in the promise chain after the thing has been seleccted - This means that the `selectImageAsync` and `selectDetectionAsync` functions must return Promises that resolve to the image or the detection (with a .loc) - There are a few APIs involved here and they all return slightly different kinds of responses (tile API, graph API, or the Mapillary Viewer itself). - This means it's easiest to break out the `_cacheImage` and `_cacheDetection` code into their own helper functions, these can work with whatever props they are passed, and can create or update caches as needed, etc.
(closes #1517) This makes it so that clicking on the detection.. actually enters select mode. This also adds some UI to show the detection details in the inspector/sidebar. - A header that says what the thing is alongside an icon - A detail section that shows some information (currently just First Seen / Last Seen) I've also cleaned up a bunch of the translations strings for working with these detected objects. I didn't realize that we've had these strings in Rapid (and probably iD) going back a long time, we just never used them.
(closes #1516) - This adds `_preventCoincident` to the Mapillary detections to avoid markers appearing on top of one another. - Also simplifies the `_preventCoincident` function that we use in various places (Mapillary, Osmose, KeepRight) - it just moves down now. - Also consistently use the word "rbush" for these spatial caches. Previously, our code had a mix of "rbush" and "rtree" in different places. But the library we use is RBush, so we will call it that.
(closes #1518) This commit adds a few more square textures that can stand in as placeholders. If a detection type doesn't have a corresponding icon in the spritesheet, we'll show a question mark icon '?' I also added the detection "type" in the sidebar so people can see what kind of sign it is. I also simplified the translation strings a bit, because we don't need a bunch of duplicate "Unknown" or "Type" strings. In situations where it's a common word used throughout Rapid, we can just have that word exist once under `inspector`.
The goal is to be able to have all our photoviewers (Mapillary, Pannellum, Kartaview, and any future photo viewers that we may add) sharing more common design and UI. These photo viewers should all respond to resizing (of both the map and the sidebar), work correctly LTR/RTL, and support having a responsive footer across the bottom that shows any options, and any attribution and metadata about the photo. Various parts of this were working/broken before, but it all works consistently now.
Also swap d3-dispatch for EventEmitter, as this is the more standard way
- Replace ugly text icons in the KartaView and Streetside viewer with nicer icons - Move the remaining few css rules into 80_app.css and remove the 60_photos.css file (this file was mostly full of legacy rules to style the old svg markers and paths) - Remove anything like `this._selectedImage` state from the services and use the `PhotoSystem.currPhoto` instead (PhotoSystem can be the owner of this now) - Remove the old practice of setting the currnt image as the `.datum()` on the photoviewer. This isn't used anymore and was just another redundant place to store the same info about which image is current.
(closes #1521) This also: - Restored the High Resolution checkbox on Streetside, which had been commented out - Removed a few strings that we haven't used in a long time, like "View on Bing Maps" - Other various cleanups
There are 2 ways the image in the viewer can change: - Changed by Rapid (e.g. user clicks a map marker, or url hash changes) In this case, it goes like: `PhotoSystem.selectPhoto` -> `selectImageAsync` -> `viewer.moveTo` -> `imagechanged` - Changed by Mapillary viewer (e.g. user clicks next/previous or arrow button) In this case, it goes like: `imagechanged` -> `PhotoSystem.selectPhoto` -> `selectImageAsync` -> `viewer.moveTo` The best place to put code to update stuff (photo footer, detections, etc) seems to be in `selectImageAsync` after the `moveTo` in the promise chain, since both scenerios end up there after the image has been loaded.
This simplifies a few things: Before: - An 'onmodechange' handler in PixiRenderer that would manage which objects in the scene should have the 'select' class. - SelectBehavior would usually enter SelectMode, but not in the case of detections, which did things differently - We had code in different places for manage the 'select' class Now: - SelectBehavior always puts the user into a Select Mode - The select modes are responsible for adding/removing 'select' class - SelectMode will also call PhotoSystem.selectDetection, which is responsible for managing the other classes (and the url) Also added some better code to ease the map into a place that should let the user view both the selected detection and the best photo to view it.
(closes #1502) This restores the Mapillary segmentation behavior to be similar to before: - all segmentations have a color now (for now mapillary green) - highlighted detection will be in yellow and have a text label - selected detection can arrive in the URL hash - lots of API stuff to know which detections are in which images - try to adjust the map to show both the detection and a best photo
Have seen some cases where even if the original geometries were uploaded with the wrong camera angles, the computed geometries have this fixed.
There is bunch going on in here, but basically I'm trying to make sure these
sorts of interactions are handled well - some were not working well before:
- the layers (what if user checks or unchecks these in the Map Data panel?)
- the viewer (what if user clicks something in the viewer,
navigates to another image? - or clicks the close ‘X’?)
- the map (what if user clicks on a detection? an image viewfield?)
- the url (do we honor the url when Rapid starts up?
does the right thing get selected? what if user changes the url?)
The approach to making this work well is that - the PhotoSystem is mainly
responsible for what is going on - it responds to changes in the layers or the
url, and any other place that wants to change the highlighted photo or
detection needs to go through the PhotoSystem to do it.
This seems to occur when we ask the Mapillary viewer to move to the same image twice. It tends to happen only when we're starting up Rapid with a selected detection in the URL hash.
The previous preset is a linear driveway preset and the icon comes from the main spritesheet, so it's drawn in a way that we can't tint the colors, making it invisible. This switches it to considering it like a parking space, which uses a tintable temaki icon.
In the url shared in #1502, this photo has a lot of segmentation information, (over 500 objects) but most of it is for object classes that we don't recognize. This commit changes it so that we only keep recognized objects and traffic signs.
This makes it easier to see what is behind the outline Also add a few clarifying comments
…etTimeout Much better to just listen for the thing we are trying to test, than to wait 50ms and see whether the thing had been called. I think this was causing the test to fail occasinally on CI. (It passes locally.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've been working for the past few weeks on improving Rapid's handling of streetview imagery.
We currently support Mapillary, Kartaview, and Bing Streetside.
All these layers see various bugfixes and improvements, they are listed below and these tickets have the details:
In addition to the above, cde9251 makes our renderer more flexible in how we "class" features in the scene. For context, we assign "classes" to certain data in the same way that we previously used CSS in Rapid v1. These classes are mostly used for stying the features as 'selected', 'hovered', 'highlightphoto', etc. The names of the classes were previously hard-coded, now they are not.