Network analysis #3773
Network analysis #3773
Conversation
|
Looks good. (Can't wait for part 2!) Don't forget to update the api break docs to reflect these changes. Something I've wondered for a while - is "Properter" a typo or a bad translation? It sounds wrong to me and I can't find any use of it anywhere else - the only results on Google relate to this code. "CostCalculator" or "Strategy" seem like better terms to me. |
|
I think "Properter" is bad translation, I also don't like it. Will follow your suggestion and change it to "Strategy". BTW, part 2 already in progress. |
Woo! - Heatmap is nearly ready to drop too |
|
@alexbruy what's the correct way in processing algs to implement custom dialog logic? Ie in the heatmap plugin there's a couple of inputs which depend on each other - change the number of columns and the number of rows changes, change the resolution and both rows/columns change. I'd like to pull this across to the processing version too, but not sure if that kind of thing is currently supported. Is there any existing algorithm which does something like this? |
|
@nyalldawson Processing has support for custom dialogs, all you need is implement custom dialog with necessary logic and return this dialog from algorithm If I'm not wrong, there are plans to add parameters dependencies to Processing, so changing parameter value will emit signal and other parameters can be updated. |
| */ | ||
| class ANALYSIS_EXPORT QgsArcProperter | ||
| class ANALYSIS_EXPORT QgsStrategy |
There was a problem hiding this comment.
I'd suggest making this a bit more precise, like QgsNetworkStrategy or QgsRoutingStrategy. There's potentially other things in QGIS which could have strategies, like labeling solutions, etc.
There was a problem hiding this comment.
Will fix it. Does it makes sense to change also names of the subclasses, e.g. to QgsNetworkDistanceStrategy?
There was a problem hiding this comment.
It's probably a good idea before they get locked in
| */ | ||
| virtual QVariant property( double distance, const QgsFeature &f ) const | ||
| virtual QVariant cost( double distance, const QgsFeature &f ) const |
There was a problem hiding this comment.
Should this method be pure virtual? I don't think anyone should be using this base class as a strategy, or the results would be totally random.
There was a problem hiding this comment.
You are right, will fix this
|
@alexbruy This is looking fantastic... very nice API clean ups! |
| #include <qgsnetworkstrategy.h> | ||
|
|
||
| /** \ingroup analysis | ||
| * \class QgsSpeedStrategy |
There was a problem hiding this comment.
This class line is what's breaking the doxygen for this class
| /** \ingroup analysis | ||
| * \class QgsSpeedStrategy | ||
| * \note added in QGIS 3.0 | ||
| * \brief Strategy for caclucating edge cost based on travel time. Should be |
There was a problem hiding this comment.
caclucating ->calculating
| * from the end point to the start point) and bidirectional or two-way | ||
| * (one can move in any direction) | ||
| */ | ||
| enum RoadDirection |
There was a problem hiding this comment.
Very nice! Can I suggest DirectionForward, DirectionReversed and DirectionBoth would be simpler names to understand?
There was a problem hiding this comment.
Actually scratch that - I think "road" should not be used here at all (since the network could be pipes, wires,... ). I'd go with something like
enum Direction
{
DirectionForward,
DirectionBackward,
DirectionBoth,
}
and also go for removing the mentions of "road" in the constructor doxygen tags. This relates to http://hub.qgis.org/issues/12158
This allows users to calculate shortest path using travel time as optimization criteria in addition to travel distance.
be consistent with other analysis stuff
Also update subclasses names and do some more refactoring.
confusion when we will have other stuff with strategies. Also rename corresponding subclasses
| * \note added in QGIS 3.0 | ||
| * \brief Strategy for caclucating edge cost based on travel time. Should be | ||
| * \brief Strategy for calcucating edge cost based on travel time. Should be |
|
Fantastic work, thanks so much! |
|
@alexbruy , thanks a lot for this. |
|
@alexbruy @nyalldawson this is great work, thanks a lot! From which QGIS version on will the |
This moves network analysis from separate library to QGIS analysis library where all other analysis routines live. Also add new properter class needed for calculating fastest route.