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

Group management enhancements #7886

Closed
wants to merge 4 commits into from
Closed

Conversation

stormcone
Copy link
Contributor

The first commit is based on @JGRennison's patch: JGRennison/OpenTTD-patches@91c5dee.
I replaced the text buttons with image buttons.

The second commit is from @KeldorKatarn's patch pack: KeldorKatarn/OpenTTD_PatchPack@3f03ec4.
I modified the loop which finds the unique orders.

The third one is also based on @KeldorKatarn's patches: KeldorKatarn/OpenTTD_PatchPack@49082fb and KeldorKatarn/OpenTTD_PatchPack@bdabd04.
I also modified it a little and added parent groups by cargo for the new groups.

group

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs a rebase :)

src/group_cmd.cpp Outdated Show resolved Hide resolved
src/group_cmd.cpp Outdated Show resolved Hide resolved
src/group_cmd.cpp Outdated Show resolved Hide resolved
src/group_gui.cpp Outdated Show resolved Hide resolved
src/group_gui.cpp Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
src/script/api/ai_changelog.hpp Outdated Show resolved Hide resolved
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Specific name" is ...a bad name. It took me a while looking at the code to realise what it was doing. Perhaps "automatically generated name" (or "autogen name" for things that want a shorter string)

In terms of commands, I wonder if it's actually worth creating a whole new command for what's essentially just CMD_CREATE_GROUP with an extra boolean flag.

CmdCreateGroup has the following bits:

p1 0-3: vehicle type
p2 0-15: parent group id

CmdCreateGroupSpecificName:

p1 0 - 19: vehicleid
p1 31: shared vehicles
p2 0-15: parent group id

Plenty of room to combine the two, IMO

@stormcone
Copy link
Contributor Author

I changed the functions'/variables' names as you suggested.

As for the command in my opinion it's a bit clearer to reading/using the code with 2 separated commands, instead of taking care of the various bits. Like if bit 1 is set, then bit xyz is this, otherwise unused; if bit 1 not set, then bit abc is that, otherwise unused, and to shift bits back and forth.

@stormcone stormcone force-pushed the auto-group branch 3 times, most recently from ffe5a2e to b2acb79 Compare June 6, 2020 10:40
@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: large This Pull Request is large in size; review will take a while labels Dec 14, 2020
@TrueBrain TrueBrain added the preview This PR is receiving preview builds label Dec 22, 2020
@DorpsGek DorpsGek temporarily deployed to preview-pr-7886 December 22, 2020 18:39 Inactive
@frosch123
Copy link
Member

There are 4 things in this PR:

  • Collapse/expand: (1st commit)
    • I like the feature and the implementation.
    • At some point someone should draw nicer icons, but considering how difficult it is to add icons and rebasing branches with them, this is for "after merge".
  • Create a new group by dragging the vehicle on the "new group" button. (half of 2nd commit)
    • There is already a similar feature: You can drag the vehicle onto an empty row.
    • Funnily this seems to be broken for a long time: once you get scroll bars, the list does not show any empty row anymore.
    • I kind of prefer the "empty row" approach, but I guess it does not hurt to have both.
  • Automatic naming of groups: (other half of 2nd commit)
    • The implementation of this has many problems:
      • A client setting and GetString() is used in command context, so this won't work in multiplayer.
      • Every client including the server will use their local client setting, and their local language for translations.
      • When joining a game, you will get the groups with the names from the server.
      • Increasing the length limit of names probably exceeds the limit for command packets. (did not check this).
    • However, independent of the implementation: I think this feature no longer makes any sense.
  • Automatic splitting of vehicles into groups by shared orders. (3rd commit)

So, as merge candidate:

  • Collapse/expand: yes
  • Create new group by dragging on "new button": most likely
  • Naming of groups by orders: definitely no
  • Splitting into groups by orders: probably not

@stormcone
Copy link
Contributor Author

* Automatic naming of groups: (other half of 2nd commit)
  
  * The implementation of this has many problems:
    
    * A client setting and GetString() is used in command context, so this won't work in multiplayer.
    * Every client including the server will use their local client setting, and their local language for translations.

Because is this causing desync or just looks "weird" when you compare the group list between the clients?

    * When joining a game, you will get the groups with the names from the server.

If you join to an existing company, you can easily recreate all of the groups. Just press the delete all groups button, than the create groups automatically button.
I play in single player mode, but I think it almost doesn't matter what the groups of other companies are called in multi player mode.

Anyway I see that it isn't "nice". But maybe could be there a place holder SCC_XXX code or something in the group names, and the clients could substitute them when the window is drawn. Or always get the group names from the server.

Or as easy alternative:

  • In multi player mode, the groups would be created without the cargo parent groups, and named by only: Town A - Town B #N. Its not that nice, but better than nothing.
  • Just hide the auto create button in multi player mode. Its a pity for the multi player guys, but single players could benefit from that.

I know these are just workarounds, but I do think this is a useful feature. I never liked manually type in the group names.

    * Increasing the length limit of names probably exceeds the limit for command packets. (did not check this).
  * However, independent of the implementation: I think this feature no longer makes any sense.
    
    * #7028 made it unnecessary to have groups per order list.

I do not agree. I do not find #7028 too useful.
For example, you have 6 identical trains: A weak initial locomotive + 5 small coal wagons. You have 2 routes. On Route 1 you have 2 trains, on Route 2 you have 4.
As time goes on, you have to replace your locomotives for some reason. On Route 1 you want a new weak locomotive and existing wagons, because they are enough. But on Route 2 you want new large coal wagons, because the mine's production has doubled during the years. But for the new wagons, you need a more powerful engine. How can #7028 help you to replace your vehicles easily?
So I think it is still necessary to have groups per order list. #7028 does not changed nothing in the case of this matter.

#7028 can be useful if you start the game in 2050 and you just use the same engine everywhere, and you have only 1 wagon to carry a specific cargo. For example a game without NewGRFs. In that case you do not have to replace your vehicles, the autorenew feature solves it.

* Automatic splitting of vehicles into groups by shared orders. (3rd commit)
  
  * Same, obsolete with #7028.

See above.

@2TallTyler
Copy link
Member

This PR has potential but hasn't been touched in two years. I think part of the problem is that it's trying to do too much.

I'm going to close this but encourage you to separate each feature and open a new PR for each one. That would be much easier to review and evaluate than having them all together like this (for example, the Preview feature is great but it's hard to track down which feature comes from which commit).

I'd like to see some of these features in the game, just help us by making them easier to review. 😃

@2TallTyler 2TallTyler closed this Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions preview This PR is receiving preview builds size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants