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

Open
wants to merge 4 commits into
base: master
from
Open

Conversation

@stormcone
Copy link
Contributor

@stormcone stormcone commented Dec 31, 2019

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

@stormcone stormcone force-pushed the stormcone:auto-group branch from 02fb5ec to 82fa7e9 Jan 2, 2020
Copy link
Member

@LordAro LordAro left a comment

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
@stormcone stormcone force-pushed the stormcone:auto-group branch 2 times, most recently from 3fead8e to b62aa43 Jan 18, 2020
@stormcone stormcone requested a review from LordAro Jan 22, 2020
@stormcone stormcone force-pushed the stormcone:auto-group branch from b62aa43 to bc3787d Feb 9, 2020
@stormcone stormcone force-pushed the stormcone:auto-group branch from bc3787d to 7202971 Apr 3, 2020
Copy link
Member

@LordAro LordAro left a comment

"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 stormcone force-pushed the stormcone:auto-group branch from 7202971 to 86b4c55 Apr 19, 2020
@stormcone
Copy link
Contributor Author

@stormcone stormcone commented Apr 19, 2020

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 stormcone:auto-group branch 3 times, most recently from ffe5a2e to b2acb79 Jun 3, 2020
@stormcone stormcone force-pushed the stormcone:auto-group branch from b2acb79 to 35dd69f Aug 29, 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

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