Directions refactoring + Show speed limit #2796
No reviewers
Labels
No labels
Accessibility
Accessibility
Address
Address
Android
Android
Android Auto
Android Auto
Android Automotive (AAOS)
Android Automotive (AAOS)
API
API
AppGallery
AppGallery
AppStore
AppStore
Battery and Performance
Battery and Performance
Blocker
Blocker
Bookmarks and Tracks
Bookmarks and Tracks
Borders
Borders
Bug
Bug
Build
Build
CarPlay
CarPlay
Classificator
Classificator
Community
Community
Core
Core
CrashReports
CrashReports
Cycling
Cycling
Desktop
Desktop
DevEx
DevEx
DevOps
DevOps
dev_sandbox
dev_sandbox
Directions
Directions
Documentation
Documentation
Downloader
Downloader
Drape
Drape
Driving
Driving
Duplicate
Duplicate
Editor
Editor
Elevation
Elevation
Enhancement
Enhancement
Epic
Epic
External Map Datasets
External Map Datasets
F-Droid
F-Droid
Fonts
Fonts
Frequently User Reported
Frequently User Reported
Fund
Fund
Generator
Generator
Good first issue
Good first issue
Google Play
Google Play
GPS
GPS
GSoC
GSoC
iCloud
iCloud
Icons
Icons
iOS
iOS
Legal
Legal
Linux Desktop
Linux Desktop
Linux packaging
Linux packaging
Linux Phone
Linux Phone
Mac OS
Mac OS
Map Data
Map Data
Metro
Metro
Navigation
Navigation
Need Feedback
Need Feedback
Night Mode
Night Mode
NLnet 2024-06-281
NLnet 2024-06-281
No Feature Parity
No Feature Parity
Opening Hours
Opening Hours
Outdoors
Outdoors
POI Info
POI Info
Privacy
Privacy
Public Transport
Public Transport
Raw Idea
Raw Idea
Refactoring
Refactoring
Regional
Regional
Regression
Regression
Releases
Releases
RoboTest
RoboTest
Route Planning
Route Planning
Routing
Routing
Ruler
Ruler
Search
Search
Security
Security
Styles
Styles
Tests
Tests
Track Recording
Track Recording
Translations
Translations
TTS
TTS
UI
UI
UX
UX
Walk Navigation
Walk Navigation
Watches
Watches
Web
Web
Wikipedia
Wikipedia
Windows
Windows
Won't fix
Won't fix
World Map
World Map
No milestone
No project
No assignees
1 participant
No due date set.
Dependencies
No dependencies set.
Reference: organicmaps/organicmaps-tmp#2796
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "RouteSegments-refactoring-and-speed-limit"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Now RouteSegments generation starts at DirectionsEngine::Generate().
But this is just preparation, when segments are not created,
but turns, street names, any new staff like speed limits are created
with indexes when it should be applied later.
Later in FillSegmentInfo all this info is unpacked to RouteSegments
and populated by segments and junctions.
This approach is overcomplicated and makes difficult adding new info,
like speed limits, by creating more and more data to pack and unpack.
Also extra memory allocations are done for intermediate steps.
Several todo were found mentioning desirability of changes.
For this reason it was decided to create RouteSegments
at DirectionsEngine::Generate() with all available info
with minimal post-processing at later stages.
#952
Add size_t const segmentsCount = routeSegments.size(); var
I have doubts about the speed of this approach .. Please, comment for now and place todo.
@ -86,0 +86,4 @@// Current speed limit. If no info about speed limit then m_speedLimit == 0.std::string m_speedLimit;std::string m_speedLimitUnitsSuffix;Can we join this into one string?
@ -29,1 +23,4 @@};void RouteSegmentsFrom(std::vector<Segment> const & segments, std::vector<m2::PointD> const & path,std::vector<turns::TurnItem> const & turns, std::vector<RouteSegment::RoadNameInfo> const & names,Add tools.cpp and move implementation?
@ -86,0 +86,4 @@// Current speed limit. If no info about speed limit then m_speedLimit == 0.std::string m_speedLimit;std::string m_speedLimitUnitsSuffix;I was done according to this:
1703c99f75/routing/following_info.hpp (L57-L58)Can you clarify why?
Just leave comment with todo?
Or disable all new code by comments?
Avoid duplicates, routeSegments.size() used ~5 times below.
@ -86,0 +86,4 @@// Current speed limit. If no info about speed limit then m_speedLimit == 0.std::string m_speedLimit;std::string m_speedLimitUnitsSuffix;Seems like we can store one string and modify FormatDistance accordingly.
Also fix the mentioned case, please.
Can we disable only this code and keep pathSegment.m_maxSpeed empty (like its not stored)?
Got it.
I will replace it by
size_t const kInvalidEnter = routeSegments.size();It will make logic easier to understand.
Sure we can.
@ -29,1 +23,4 @@};void RouteSegmentsFrom(std::vector<Segment> const & segments, std::vector<m2::PointD> const & path,std::vector<turns::TurnItem> const & turns, std::vector<RouteSegment::RoadNameInfo> const & names,OK
We can cache result of
routing::LoadMaxspeeds(handle);
If your concern is about performance.
Most likely it will be the same for all segments, right?
Leave todo and I will think about the optimal strategy. We already have speeds cache in GeometryLoader (GeometryLoaderImpl), just need to access it here somehow.
@ -86,0 +86,4 @@// Current speed limit. If no info about speed limit then m_speedLimit == 0.std::string m_speedLimit;std::string m_speedLimitUnitsSuffix;The reason for separation is that in GUI values and units are shown now (and can be shown in the future) in different ways (fonts and positions).
In fact units for CURRENT speed already exist,
but current speed is calculated and stored separately (at device specific level),
so I decided to provide units here for asserted comparison.
OK
@ -29,1 +23,4 @@};void RouteSegmentsFrom(std::vector<Segment> const & segments, std::vector<m2::PointD> const & path,std::vector<turns::TurnItem> const & turns, std::vector<RouteSegment::RoadNameInfo> const & names,Can you please add to XCode project
routing/routing_tests/tools.cpp
@ -86,0 +86,4 @@// Current speed limit. If no info about speed limit then m_speedLimit == 0.std::string m_speedLimit;std::string m_speedLimitUnitsSuffix;Hm, well ok, keep as is.
Added minor comments.
@ -33,4 +29,1 @@uint32_t turn_index = base::asserted_cast<uint32_t>(junctions.size() - 1);turnsDir.emplace_back(TurnItem(turn_index, CarDirection::ReachedYourDestination));double constexpr kMergeDistMeters = 15.0;We use 'k' prefix for the compile time constants, but here 'invalidEnter' is better.
2 spaces for tab
; is redundant.
Well, we don't use this notation. Please, move ");" to the end of prev line.
And seems like RouteSegment() is not needed for emplace_back (but not sure).
@ -33,4 +29,1 @@uint32_t turn_index = base::asserted_cast<uint32_t>(junctions.size() - 1);turnsDir.emplace_back(TurnItem(turn_index, CarDirection::ReachedYourDestination));double constexpr kMergeDistMeters = 15.0;OK
OK
OK
OMG, it's really not necessary!
Changed here and in routing/directions_engine.cpp
@vng you're too fast )
@ -467,21 +467,25 @@ void DrawWidget::SubmitFakeLocationPoint(m2::PointD const & pt)if (m_framework.GetRoutingManager().IsRoutingActive()){// Immidiate update of of position in Route. m_framework.OnLocationUpdate is too late.Why updating it in m_framework.OnLocationUpdate is too late? It would be great to have a more detailed explanation here.
Also, should the update in
m_framework.OnLocationUpdatebe removed, because we already do it here?@ -35,4 +29,3 @@double constexpr kMergeDistMeters = 15.0;// For turns that are not EnterRoundAbout/ExitRoundAbout exitNum is always equal to zero.// If a turn is EnterRoundAbout exitNum is a number of turns between two junctions:Generally, I would prefer to leave std:: and remove
using namespace std;from the code, at least from the namespace level (using it locally in function bodies is ok). It could make surprises with Unity builds.@ -85,1 +85,4 @@//@}// Current speed limit. If no info about speed limit then m_speedLimit == 0.std::string m_speedLimit;Should it have a default constructor = "0"?
@ -117,3 +118,4 @@m_poly.GetDistFromCurPointToRoutePointMeters() / curSegSpeedMPerS);}void Route::GetCurrentSpeedLimit(SpeedInUnits & speedLimit) constWhy not returning/checking the value? What if IsValid() == false?
@ -0,0 +10,4 @@m_session->SetOnNewTurnCallback([this]() { ++m_onNewTurnCallbackCounter; });}void RouteSegmentsFrom(std::vector<Segment> const & segments, std::vector<m2::PointD> const & path,Why not use 4 separate but simple and clear functions instead of this monster one?
@ -216,3 +237,4 @@}}UNIT_TEST(TestIsLaneWayConformedTurnDirection){} instead of empty containers.
{}
@ -261,1 +281,3 @@{2, CarDirection::ReachedYourDestination}};vector<turns::TurnItem> turns ={{1, CarDirection::GoStraight},{2, CarDirection::TurnLeft},{}
@ -467,21 +467,25 @@ void DrawWidget::SubmitFakeLocationPoint(m2::PointD const & pt)if (m_framework.GetRoutingManager().IsRoutingActive()){// Immidiate update of of position in Route. m_framework.OnLocationUpdate is too late.I need RoutingSession::OnLocationPositionChanged to be executed right now
to get updated FollowingInfo state. Previously you could see only old state,
so it prevented you from normal visual debug.
Framework::OnLocationUpdate calls RoutingSession::OnLocationPositionChanged later indirectly
via callbacks. BTW it is called 10+ times. I can't understand if it's normal.
@vng maybe you have any idea?
Just put breakpoint in master at RoutingSession::OnLocationPositionChanged
and check how many times it is called.
@ -35,4 +29,3 @@double constexpr kMergeDistMeters = 15.0;// For turns that are not EnterRoundAbout/ExitRoundAbout exitNum is always equal to zero.// If a turn is EnterRoundAbout exitNum is a number of turns between two junctions:Can you clarify this?
Currently almost all routing/*.cpp files have
using namespace std;at global or namespace level.@ -85,1 +85,4 @@//@}// Current speed limit. If no info about speed limit then m_speedLimit == 0.std::string m_speedLimit;Outdated comment. Changed to
// If no info about speed limit then m_speedLimit == "" and m_speedLimitUnitsSuffix == "".@ -467,21 +467,25 @@ void DrawWidget::SubmitFakeLocationPoint(m2::PointD const & pt)if (m_framework.GetRoutingManager().IsRoutingActive()){// Immidiate update of of position in Route. m_framework.OnLocationUpdate is too late.What do you mean "called 10+ times"? Can you please show some logs? That's not normal.
@ -35,4 +29,3 @@double constexpr kMergeDistMeters = 15.0;// For turns that are not EnterRoundAbout/ExitRoundAbout exitNum is always equal to zero.// If a turn is EnterRoundAbout exitNum is a number of turns between two junctions:Global level: Strictly NO. It may break the unity build.
Namespace level: better NO than YES.
Function level: YES if it makes the code cleaner.
@ -467,21 +467,25 @@ void DrawWidget::SubmitFakeLocationPoint(m2::PointD const & pt)if (m_framework.GetRoutingManager().IsRoutingActive()){// Immidiate update of of position in Route. m_framework.OnLocationUpdate is too late.Didn't check yet, but seems like this is the position extrapolation algorithm for the smooth location events.
@ -467,21 +467,25 @@ void DrawWidget::SubmitFakeLocationPoint(m2::PointD const & pt)if (m_framework.GetRoutingManager().IsRoutingActive()){// Immidiate update of of position in Route. m_framework.OnLocationUpdate is too late.And this is normal for usual use case :) But a bit frustrated for desktop debugging
@ -35,4 +29,3 @@double constexpr kMergeDistMeters = 15.0;// For turns that are not EnterRoundAbout/ExitRoundAbout exitNum is always equal to zero.// If a turn is EnterRoundAbout exitNum is a number of turns between two junctions:So we need to fix all cases like?
4e75b5bea7/routing/async_router.cpp (L13)@ -467,21 +467,25 @@ void DrawWidget::SubmitFakeLocationPoint(m2::PointD const & pt)if (m_framework.GetRoutingManager().IsRoutingActive()){// Immidiate update of of position in Route. m_framework.OnLocationUpdate is too late.Will leave extended comment here.
@ -117,3 +118,4 @@m_poly.GetDistFromCurPointToRoutePointMeters() / curSegSpeedMPerS);}void Route::GetCurrentSpeedLimit(SpeedInUnits & speedLimit) constOK
@ -216,3 +237,4 @@}}UNIT_TEST(TestIsLaneWayConformedTurnDirection)OK
OK
@ -261,1 +281,3 @@{2, CarDirection::ReachedYourDestination}};vector<turns::TurnItem> turns ={{1, CarDirection::GoStraight},{2, CarDirection::TurnLeft},OK
@ -0,0 +10,4 @@m_session->SetOnNewTurnCallback([this]() { ++m_onNewTurnCallbackCounter; });}void RouteSegmentsFrom(std::vector<Segment> const & segments, std::vector<m2::PointD> const & path,I would prefer creation of test routeSegments in one line.
@ -35,4 +29,3 @@double constexpr kMergeDistMeters = 15.0;// For turns that are not EnterRoundAbout/ExitRoundAbout exitNum is always equal to zero.// If a turn is EnterRoundAbout exitNum is a number of turns between two junctions:Well, no need to check all modules now, but fix least those you are modifying now. Put using under 'routing' namespace.