Validation and change performance #6140
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
- This is to make difference comparisons easier - The only nodes that didn't have a `loc` were fake nodes we made for testing - So this commit also fixes the `osmIntersection` code and tests.
- Make sure all state variables prefixed with `_` - Add explicit `init`/`reset` methods (graph/entity refs should never persist through a save to OSM) - Thinking of how best cache validation results
bhousel
force-pushed
the
validation_and_change_perf
branch
from
0aa8f65
to
82a9375
Compare
Givnig them names makes it easier to understand in the profiler
Since d5e4272, the tree head graph will not update if only tags have changed - it requires an addition, deletion, or geometry change. This makes the tree a little more efficient, but we do need to make sure to return the current entities to the caller.
(closes #4890) This lets iD request needed tiles outside of the viewport, for example to properly straighten lines or validate features that may have unloaded connections.
What could happen was: - user could right click on a line - this would trigger `disabled()` checks for each operation buttons - the line was not fully downloaded, so would return `disabled()` 'not_downloaded' (and also start download of the missing tiles) - then the tooltip would pop into existence, calling `tooltip()` - which calls `disabled()` again - but this time it's fine and the `disabled()` is false - so you'd see a greyed out button but the tooltip said everyting is ok and you can click the button anyway I fixed this by just caching the disabled test. This is probably ok anyway because these tests can be expensive, and then the user will see a consistent message like "The line is not yet fully downloaded". If the user clicks off the line and back onto it, iD will reenter select mode, rebuild the menu, redo the disabled test, and they will see the button enabled.
I think this doesn't completely work.. It reduces the amount of unnecessary DOM changes, but there are still some more.
Reference will show on clicking info button, and can contain more useful information than a tooltip can.
It just doesn't fit in with all of the other validations that work on entities, and it's not an actionable warning anyway. #6140 (comment)
bhousel
referenced
this pull request
Apr 20, 2019
bhousel
force-pushed
the
validation_and_change_perf
branch
from
f672cb5
to
3fe9d75
Compare
(Can probably find a better way to make these responsive later)
`map.editable` is hot code because it's called frequently by the `isHiddenX` tests in `features.js`. It's much more efficient to just ask the osm layer whether it is enabled, than to use D3 to find that layer in the DOM and check whether it's classed `disabled`
This is important to make the turn restriction editor work
|
This doesn't do everything that I would like it to, but it's good to merge now. The validator is really impressive, and I'd love for people to try it out in http://preview.ideditor.com/master We're going to continue to do a lot of testing this week, and see if we can add a few more improvements before the next release ✨ |
This was referenced Apr 23, 2019
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.
This PR will attack a bunch of related performance issues..
Basically we can do slightly more work in
coreDifferenceto track what kind of stuff got changed in the difference, so that other parts of the iD code can use this information to avoid expensive processing.Avoid rerendering the sidebar so frequently #6086 - avoid rerendering the sidebar for geometry-only changes
check
coreTree, which can use the improved difference to avoid expensive delete/reinsert of entity bounding boxesPerformance degraded after validation was added #6054 / Re-validate features only as-needed due to further edits #5901 / Validation: Live validation hits too soon #5898 - validator performance
We are currently validating all the changes in the user's edit history every time the history changes significantly - and we should instead run validation per-entity.
Option to browse all nearby issues, not just those for edited features #5906 - validate unedited features as well as edited features
I accidentally implemented this today, and was surprised by how well it worked. Needs some UI to let the user opt in to it.
False warning for highways connected only to unloaded highways #5938 - false warning about disconnected features that extend beyond the loaded map
These are already bad, but we see them a lot once we try validating all the features per Option to browse all nearby issues, not just those for edited features #5906
'Straighten' on long ways can separate junction points #2248 / Actions should be able to request additional data #4890 - should really fix these old bad bugs once we have the code to let other parts of iD code ask the osm service what tiles have been loaded per False warning for highways connected only to unloaded highways #5938
considering: avoid rendering some geometry in map.js#draw
This depends knowing which tags affect style. e.g.
highwaycontrols the color of lines,onewaycontrols oneway markers, etc. There is no great solution to this problem, since style still lives in both JS and CSS for now.Add ability to flag a warning as a false positive. Something like "Don't warn me about this again"
Add lots of tests