andythenorth wrote:

This is a request for expand/collapse, similar to advanced settings window (with +/- icons)? That's my interpretation anyway :)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14534

Milsa wrote:

Look at my Windows's "My computer" window with this functionality (left bottom).

Attachments

• group-packed.png (90.96 KiB)
• group-unpacked.png (85.10 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14536

3298 wrote:

Is there any other reasonable interpretation?

Anyway, I have a patch for this. It's not even new, but only the recent activity prompted me to dig it up.
- The first part is just a preparatory change (fix?) that makes the profit icon and vehicle number for groups also count vehicles in subgroups. It's not strictly needed, but it makes sense, especially when subgroups can be hidden by collapsing their parent:
- - Imagine a group with no vehicles directly in it, but with subgroups that have vehicles. With the current code the group will show 0 vehicles and the N/A icon for group profit.
- - Now imagine the same scenario, but with the group collapsed (hiding the non-empty subgroup). If you don't select the group to list the vehicles in it, the left panel will give the false impression that there are no vehicles here.
- - Since r27822 the then-new detailed group profit info (which does take vehicles in subgroups into account) can contradict the profit icon of the selected group.
- The second part is the actual patch for collapsible subgroups. Most of it should be pretty self-explanatory or commented. (If not, ask me.)

Attachments

• groupcollapse-prep.diff (2.06 KiB)
• groupcollapse.diff (6.71 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14538

andythenorth wrote:

Applying groupcollapse.diff against svn r27895:

patching file src/group_cmd.cpp
Hunk # 1 FAILED at 274.
1 out of 1 hunk FAILED -- saving rejects to file src/group_cmd.cpp.rej
patching file src/group_gui.cpp

***************
*** 274,279 ****
{
this->owner = owner;
this->parent = parent;
}

Group::~Group()
--- 274,281 ----
{
this->owner = owner;
this->parent = parent;
+
+ this->folded = false;
}

Group::~Group()

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14591

andythenorth wrote:

With help, I compiled with both patches applied.

The expand/collapse so far works exactly as I would expect. I didn't try and break it though :)

The count doesn't work right for me. It seems to only count inside child groups, and not in self. Image attached.

Attachments

• group-counting.png (52.65 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14592

3298 wrote:

Re: failing to apply: my bad, should have rebased on trunk before posting. I'm maintaining several feature branches locally, the one dealing with groups currently contains my patches for #6491, #5977, and the two for this one, in that order. The first of these adds a line to that constructor as well, which shows up in the context (the one about parent). Your patching utility complained when it couldn't find that context. (Mine does too, just tested.)

Re: vehicle count: actually, it seems to be the other way around. You have two vehicles in Group 2 (vehicle count matches), and three directly in Group 1. The count should say 5, but your picture says 3. In trunk it would say 3 too. That's what the preparatory part should take care of, are you sure you have that applied? Does that patch work for you on its own? I'll check that myself in a moment, but I was sure I had that working as I intended...

Note for future readers: the preparatory patch should have been #6189, but I wasn't aware of that ticket when I posted it here. I wrote a note over there already when Andy brought it to my attention by digging it up.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14593

andythenorth wrote:

[double post somehow]

Attachments

• group-counting.png (52.65 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14594

3298 wrote:

Wait a minute, I took a closer look at the vehicle counts and spotted the 7 in the All trains line and the 2 in the Ungrouped trains line, so there should be none left to be directly in Group 0. But it displays 3 (could be inherited from Group 1, as it should). So there's something weird going on.

And I think I know exactly what. Changing the group hierarchy doesn't update the vehicle counts of the past parent and future parent of the moved group. It should. Fixing...

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14595

3298 wrote:

Okay, that bug is squashed. While fixing it, something else occurred to me:
Out of the six member variables of GroupStatistics, my prep patch changes three to be affected by their copies in subgroups' stats. They are now properly handled when changing the hierarchy (the changes in CmdAlterGroup are responsible for this). The three remaining ones seem to deal with Autoreplace (autoreplace_defined, autoreplace_finished, num_engines), and I don't know how the introduction of hierarchical groups affected Autoreplace. Depending on how it works, there may be a trunk bug similar to the patch bug I just fixed, but that needs further investigation.

The attached files are now properly based on current trunk. No more manual hacks needed to make them apply. ;)

Attachments

• groupcollapse-prep.diff (3.39 KiB)
• groupcollapse.diff (6.72 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14600

andythenorth wrote:

Applied and tested the patches separately, in sequence (to trunk r27895).

1. groupcollapse-prep also adds the profit display. The profit display would address Vehicle Groups: summarised income & profit at the vehicle lists by groups #5334 in a way I hadn't thought of, but it needs to be split to a separate patch if there's any chance of persuading a dev to review this :)
2. groupcollapse-prep - the counts work for me, I tried to break it, couldn't find any edge cases.
3. groupcollapse works for me, I tried to break it, couldn't find any edge cases. If an expanded group is the last item in a long group list, collapsing it will cause the group position to shift away from the cursor as the window scroll collapses. That could cause player to click on wrong group. I don't think that's worth addressing.

I didn't test with autoreplace at all, nor in MP. I also don't know if AIs rely on the current group counts, and might be confused by the inclusion of the subgroups.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14602

3298 wrote:

As far as I understand (from looking at the code), scripts (including AIs) don't even have access to group stats. They don't have access to the group hierarchy either, so if an AI uses groups, it can only create parent-less groups (at least, without help from players using the company changing cheat, which they probably can't count on). For that kind of hierarchical structure, this patch changes nothing, so AIs are not affected.

Regarding MP, the changes brought by the first patch are structured such that it should remain bug-free if it was (I simply replaced updates to a single group by updates to a group and all parents). The changes in the second patch are designed to be purely client-side; I avoided the functions run in command scope. If you look at the diff, that patch only adds a single line to group_cmd.cpp, which ensures that the "folded" member of a Group is initialized in the constructor. Thus I'm pretty sure this is MP-safe.

Autoreplace would be a separate issue. I only brought this up here so the thought isn't lost.

1. About Vehicle Groups: summarised income & profit at the vehicle lists by groups #5334: I didn't do anything about that. The detailed info for the currently selected group was implemented in r27822, and I didn't touch it (by the way, 5334 links to Group info #1042, which was about that detailed info); the group profit icons are ancient (another ticket linked to in 5334, Group profit in Vehicle List #4037, was closed after they were implemented), and I only changed the underlying calculation along with the group count. I don't think splitting this profit_last_year and num_profit_vehicle part out of the patch is worth it. Besides, 5334 is older than hierarchical groups (which introduced the "issue" I'm fixing, if you want to call it that) but newer than the group profit icons, so its author apparently wanted more than those icons.
2. Nice.
3. Well, all programs I know of do that. Windows Explorer, its KDE equivalent Dolphin, lots of save / load dialogs with a panel showing the directory hierarchy, websites with scrollbars where stuff is conditionally shown, etc. Even some other places in OpenTTD behave like that, most prominently the advanced settings window. If you call that an issue, not shifting the groups downwards (i.e. the opposite option) could equally be called an issue because you couldn't reach that empty space below the groups (where the now-hidden groups were displayed) normally via the scrollbar. So it's a design decision that's common as well as consistent within OpenTTD. There is a third option (delaying the scrollbar update that causes this down-shift until the mouse leaves the panel, inspired by the tab bar in current desktop Firefox when closing tabs), but that's hard to implement and certainly a separate patch. Oh, and changes to the group count or group order by other players can already interfere in a similar way, which we can't really do a lot about either, other than maybe stealing the idea of accidental click protection from modern OS window managers (i.e. ignoring clicks if what you're clicking on is different than what you would have clicked on a few milliseconds earlier). That would be a separate (hard) patch too.
   TL;DR: I agree, we're done here.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14611

andythenorth wrote:

My comments about group profit display were my error, thanks :)

I was comparing profit display in the group list with 1.7.1 release of OpenTTD which lacks it. I've closed # 5334 as 'implemented' due to r27822.

I'll try and persuade someone to review this.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14613

Alberth wrote:

I spend some time studying this patch (in particular, the first patch only at this time), and it seems to work, but I don't understand the solution.

You recursively update last-year profit, and number of profit vehicles. What doesn't get changed is current profit, and number of engines. Yet those must have a similar solution that you have added for the former two variables.
I don't like having 2 different mechanisms in the same window to achieve the same thing (printing cumulative values for data in groups). Could you explain why you added a different way for last-year profit and number of profit vehicles? As it is now, it doesn't look very logical and simple to me.

There are 2 small issues with the patch itself.
First issue is behind an 'else' there is always a curly bracket.
You do
if (...) {
...
} else for (...) {
}
Code style says
if (...) {
...
} else {
for (...) {
...
}
}

The second issue is with several lines comments under each other.
You do
/* .... /
/ .... ... /
Code style says
/ ....
* .... ... */

I haven't looked closely enough at the 2nd patch to say anything useful about it now.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14656

3298 wrote:

- "What doesn't get changed is current profit..."
Not sure what you mean. There is no profit_this_year in GroupStatistics (which contains the only relevant numbers), only profit_last_year. If this is aimed at the group info panel, blame r27822 which calculates this on the fly (somewhere in the window's DrawWidget function IIRC, along with group occupancy, which looked harder to update incrementally). A cleanup patch that unifies this behavior may be a good option, but I didn't think it would be necessary. Will look into it tomorrow.
- "... and number of engines."
As you discovered on IRC, it's also calculated on the fly, but this time in a separate function in group_cmd.cpp. I already pointed out that there's a place that should call it (fixed in #5978; repeating here for future readers, and perhaps as a reminder :p ). A cleanup patch should probably fold that into the recursive update as well. I didn't change it because it was working and because it is part of Autoreplace which I didn't want to touch at the time because I knew nothing about its inner workings (but then I did it anyway for 5978, so whatever).

- "Could you explain why ..."
A few decisions. Possibly bad ones, but I'll try to explain.
When changing things (generally, not just in OpenTTD), I always look at the performance impact of my changes, so I opted for a more "aggressive" updating mechanism for the variables relevant to my cause rather than the on-the-fly way because that would involve several loops over ALL groups (one loop per recursion level) to add up all the per-group variables for display. There are definitely more redraws than data updates (data updates trigger a redraw, but redraws can be needed without a data update), and the average aggressive update should touch less groups than the average on-the-fly redraw, so the aggressive update should be faster.
I didn't touch the group info panel or the num_engines code because "don't fix it if it ain't broken". On the other hand, you mention inconsistency (which can be an issue, i.e. "it's broken"), sooo ... yeah. I'll look for a way to fix that. Thanks for the feedback. :)

I'll fix the code style. For my own projects I have a similar code style, and these are some of the differences, so that's where I went wrong.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14657

3298 wrote:

"Tomorrow" ... real life interfered.
The code style has been fixed. I inserted two cleanup patches: the first one (-prep2) deals with num_engines, the second one deals with the info panel, while introducing a few new members in GroupStatistics to achieve that goal (making sure that they are properly updated was painful though, and I had to touch a lot of files). The final patch remained unchanged.
The second cleanup patch also fixes two tiny bits I found while tracing the occupancy part (which, by the way, was easier to edit than I anticipated): the member Vehicle::trip_occupancy was accessed without "this->", and it was declared as int8, even though the values written into it were coming from a function returning uint8, and its only use (group info panel) was adding to a uint32. I changed it to uint8 for consistency.

Attachments

• groupcollapse-prep.diff (3.41 KiB)
• groupcollapse-prep2.diff (3.03 KiB)
• groupcollapse-prep3.diff (14.60 KiB)
• groupcollapse.diff (6.72 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14713

3298 wrote:

Due to the uncomfortably large amount of changes I also took a stab at establishing consistency the other way: changing my patch to work the way the existing code does. It's certainly easier, though the multiple loops over all groups just to re-draw the group's vehicle count still doesn't feel good. I'll let you decide, just giving you some options.

Attachments

• groupcollapse-prep.diff (5.23 KiB)
• groupcollapse.diff (6.72 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6053#comment14714

3298 has stopped working on this due to ethical concerns about github: https://www.tt-forums.net/viewtopic.php?f=33&t=83025

I do think it's a legit improvement.