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

Feature: Option to group vehicle list by shared orders #7028

Open
wants to merge 4 commits into
base: master
from

Conversation

@btzy
Copy link
Contributor

btzy commented Jan 8, 2019

I added a "group by" dropdown to all vehicle lists to display one row per shared order, instead of one row per vehicle. I've tested it with all vehicle types, as well as with the "vehicle groups" window. When grouped by shared orders, clicking on a row will open the "shared orders of X vehicles" window.

Rationale: Quite often, we are concerned only with "routes" instead of individual vehicles. This feature highlights the important information (i.e. the routes) while reducing the importance of non-important information (i.e. the individual vehicles).

Extension: It is possible to have a third option - to group by vehicle group. As vehicle groups are mutually exclusive, it should be relatively simple to implement this extension. However, I'm not sure of its usefulness and I didn't want to make this PR too big.

Note: The new dropdown is available in the "shared orders of X vehicles" window too. Though it is of not much use, I left it in for consistency. The dropdown will be useful for the extension described above.

This is as per my post on TT Forums: https://www.tt-forums.net/viewtopic.php?f=32&t=84587&sid=fa68e80118896fca8f275a70d5cf6045

Some screenshots:
image
image
image
image

@btzy btzy force-pushed the btzy:station-vehicle-grouping branch from a02f906 to 7d4722c Jan 8, 2019
Copy link
Member

LordAro left a comment

General idea is great, thanks for contributing! It's quite a large diff with lots of changes all together, do you think it would be feasible to split it into multiple logical commits?
I wonder about the fairly significant use of C++11 stuff as well. Technically we should be all fine for it, but...

src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/widgets/group_widget.h Outdated Show resolved Hide resolved
src/widgets/vehicle_widget.h Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
@btzy
Copy link
Contributor Author

btzy commented Jan 11, 2019

@LordAro Maybe I can split it into two commits: The first commit would split the original this->vehicles into this->vehicles and this->vehgroups. And the second commit would introduce GB_NONE and GB_SHARED_ORDERS. Would this be okay?

@btzy btzy force-pushed the btzy:station-vehicle-grouping branch 7 times, most recently from a31262a to 70ab77f Jan 11, 2019
@LordAro LordAro dismissed their stale review Jan 20, 2019

fixed

@LordAro LordAro requested a review from PeterN Feb 10, 2019
src/vehicle_gui.cpp Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Show resolved Hide resolved
@btzy btzy force-pushed the btzy:station-vehicle-grouping branch 3 times, most recently from 34cef8f to dbcac10 Feb 21, 2019
@PeterN
Copy link
Member

PeterN commented Feb 28, 2019

Seems confusing to have both explicit groups and this. Not sure where they should fit together.

@PeterN
Copy link
Member

PeterN commented Feb 28, 2019

This doesn't compile currently:

src/vehicle_gui.cpp:204:73: error: initialized lambda captures are a C++14 extension [-Werror,-Wc++14-extensions] const Vehicle **end = std::find_if_not(begin, this->vehicles.End(), [first_shared = (*...

Plus it would be useful to rebase to master.

@btzy
Copy link
Contributor Author

btzy commented Feb 28, 2019

Seems confusing to have both explicit groups and this. Not sure where they should fit together.

My original motivation was that it was difficult to find a correct vehicle (for cloning / Ctrl-clicking) in a long list. I tend to play with lots of vehicles, and without this patch I would have to click vehicles at random and open their order lists to see if I've gotten the correct one.

Grouping by shared orders also allows certain contextual actions (on the vehicle list) that explicit groups can't have, such as opening the "Shared orders of X vehicles" window, opening the order list, or Ctrl-clicking for cloning.

Since both explicit groups and shared orders already exist in OpenTTD and solve orthogonal concerns (explicit groups being arbitrarily user-defined and shared orders being defined by having the same route), maybe it would reduce confusion to have a third option in the "group by" dropdown that groups by the explicit (user-defined) group? This would probably emphasise the distinction between explicit groups and shared orders. (If this is done, maybe "user-defined group" is a better term than "vehicle group".)

@Eddi-z
Copy link
Contributor

Eddi-z commented Feb 28, 2019

I would maybe even go further and include other automatic group-like features like "all vehicles visiting <station>"

@btzy btzy force-pushed the btzy:station-vehicle-grouping branch 2 times, most recently from a578902 to d87bfd3 Feb 28, 2019
@btzy
Copy link
Contributor Author

btzy commented Feb 28, 2019

I would maybe even go further and include other automatic group-like features like "all vehicles visiting "

I'm not sure how this could be a group-like feature. Vehicles can visit multiple stations, so this kind of grouping doesn't seem to be mutually exclusive? Looks more like a filter-like feature to me.

@btzy
Copy link
Contributor Author

btzy commented Feb 28, 2019

This doesn't compile currently:

src/vehicle_gui.cpp:204:73: error: initialized lambda captures are a C++14 extension [-Werror,-Wc++14-extensions] const Vehicle **end = std::find_if_not(begin, this->vehicles.End(), [first_shared = (*...

Plus it would be useful to rebase to master.

Sorry about that, now fixed and rebased to master.

@PeterN
Copy link
Member

PeterN commented Feb 28, 2019

Thanks, now able to test it :-)

src/group_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui_base.h Outdated Show resolved Hide resolved
@PeterN
Copy link
Member

PeterN commented Mar 25, 2019

Could the group-by stuff be hidden/disabled when in the grouped-by vehicle list?

@btzy
Copy link
Contributor Author

btzy commented Mar 25, 2019

Could the group-by stuff be hidden/disabled when in the grouped-by vehicle list?

I'm not sure which list you are referring to... do you mean the "Shared orders of X vehicles" list?

@PeterN
Copy link
Member

PeterN commented Mar 25, 2019

Yes, that particular one. I'm not sure if it makes sense or not to be have to have it grouped by shared orders, as it already is shared orders. On the other hand, if other group-by schemes are added it would again make sense. Hmm!

@btzy
Copy link
Contributor Author

btzy commented Mar 26, 2019

Here's how it looks when disabled:

image

@btzy btzy force-pushed the btzy:station-vehicle-grouping branch 3 times, most recently from ef2cf9d to 9123cf3 Mar 28, 2019
@btzy
Copy link
Contributor Author

btzy commented Mar 28, 2019

@PeterN I've made the changes as you suggested, and also changed a few things related to the recent commits that replaced SmallVector with std::vector.

@michicc michicc dismissed their stale review Mar 30, 2019

Outdated.

@stale
Copy link

stale bot commented Apr 29, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Apr 29, 2019
@btzy btzy force-pushed the btzy:station-vehicle-grouping branch from 9123cf3 to 1437488 May 1, 2019
@stale stale bot removed the stale label May 1, 2019
@btzy
Copy link
Contributor Author

btzy commented May 1, 2019

@PeterN Can you please review this PR? Your stalebot is reminding me about this...

@LordAro LordAro requested a review from PeterN May 2, 2019
@LordAro LordAro dismissed PeterN’s stale review May 2, 2019

resolved

src/vehicle_gui.cpp Outdated Show resolved Hide resolved
src/vehicle_gui.cpp Outdated Show resolved Hide resolved
&VehicleLengthSorter,
&VehicleTimeToLiveSorter,
&VehicleTimetableDelaySorter,
BaseVehicleListWindow::VehicleGroupSortFunction * const BaseVehicleListWindow::vehicle_group_none_sorter_funcs[] = {

This comment has been minimized.

Copy link
@LordAro

LordAro Aug 17, 2019

Member

vehicle_group_none_sorter_funcs is a horrible name, we must be able to come up with something better

This comment has been minimized.

Copy link
@btzy

btzy Dec 13, 2019

Author Contributor

It's for consistency with vehicle_group_shared_orders_sorter_funcs; these are modes of the enum GroupBy, which uses GB_NONE and GB_SHARED_ORDERS. I can't think of a better name that would make sense here; could you provide a suggestion?

src/vehicle_gui.cpp Outdated Show resolved Hide resolved
typedef GUIList<const Vehicle*> GUIVehicleList;

struct GUIVehicleGroup {
VehicleList::const_iterator vehicles_begin; ///< Pointer to beginning element of this vehicle group.
VehicleList::const_iterator vehicles_end; ///< Pointer to past-the-end element of this vehicle group.

This comment has been minimized.

Copy link
@LordAro

LordAro Aug 17, 2019

Member

storing iterators like this scares me a little - there's no way to know if they've been invalidated

This comment has been minimized.

Copy link
@btzy

btzy Dec 13, 2019

Author Contributor

They may be slightly dangerous, but I think it's justified in this scenario.

Only BaseVehicleListWindow::BuildVehicleList() will modify the underlying VehicleList, which will cause the iterators to be invalidated. I'll add some comments around to make it clear that vehgroups requires stability of the underlying vehicle list.

I didn't want to make GUIVehicleGroup contain a std::vector because of these reasons:

  • The current design reinforces the notion that the grouping option is just a "view" over the actual vehicle list.
  • So that we can make the optimisation mentioned in vehicle_gui.cpp:185 (This can be optimized so that merely changing the "group by" dropdown value does not need to call GenerateVehicleSortList again.) in the future, if found to be necessary.
  • So that we won't have so many tiny vectors floating around the heap.

It might be possible to do something like:

template <typename T>
class GUIGroupableList<T> : public GUIList<T>;

typedef GUIVehicleGroupList GUIGroupableList<const Vehicle *>;

GUIGroupableList would contain the grouping mechanism, as well as the current struct GUIVehicleGroup (but perhaps made generic as appropriate). This would effectively combine BaseVehicleListWindow::vehicles and BaseVehicleListWindow::vehgroups. However, this would add quite a bit of code, and designing GUIGroupableList in a generic manner will require some kind of accumulation mechanism (e.g. to accumulate display_profit_this_year, display_profit_last_year, and age) (perhaps we have GUIGroupList<U, Func, T> where U is the accumulated type (i.e. a struct that contains display_profit_this_year, display_profit_last_year, and age fields), T is the original type (i.e. const Vehicle *), and Func is a binary function such that U is constructible from std::invoke_result<Func, U, T>). This feels less easily understood as compared to the current design.

src/vehicle_gui_base.h Outdated Show resolved Hide resolved
src/vehicle_gui_base.h Outdated Show resolved Hide resolved
src/group_gui.cpp Outdated Show resolved Hide resolved
@btzy
Copy link
Contributor Author

btzy commented Sep 17, 2019

@LordAro I'm a bit busy these few weeks, I'll take a look at your comments sometime soon!

@btzy btzy force-pushed the btzy:station-vehicle-grouping branch 5 times, most recently from 05775cd to fea567e Dec 13, 2019
@btzy btzy requested a review from LordAro Dec 13, 2019
@LordAro LordAro dismissed their stale review Dec 23, 2019

needs rereview

@LordAro
Copy link
Member

LordAro commented Dec 23, 2019

Given the recent large codechange in #7864 , can you rebase and check everything still works? I'm almost surprised there isn't any conflicts...

EDIT: Well, there are definitely conflicts now

@btzy btzy force-pushed the btzy:station-vehicle-grouping branch from fea567e to c5f1a6c Dec 27, 2019
@btzy
Copy link
Contributor Author

btzy commented Dec 27, 2019

@LordAro Fixed conflicts and rebased on master, this should be ready for re-review now. I didn't implement a similar behaviour for Ctrl-clicking on a vehicle, because it is possible for two vehicles that share orders to be in different groups.

Note: I find it weird that in the group GUI, if I Ctrl-click a vehicle, the selected group changes, AND the vehicle view window is shown. But in the station vehicle GUI, if I Ctrl-click a vehicle, the group GUI opens to the selected vehicle's group, but the vehicle view window doesn't open. Seems like an inconsistency... This isn't really related to this PR; I guess there might be reasons for this behaviour...

btzy added 4 commits Jan 11, 2019
This is in preparation for the new UI feature that allows grouping by shared orders.
This applies to all kinds of vehicle lists, as well as the "vehicle groups" window.
@btzy btzy force-pushed the btzy:station-vehicle-grouping branch from c5f1a6c to 5a19beb Dec 27, 2019
@LordAro LordAro added the stale label Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.