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

Template DoCommand and friends #9725

Merged
merged 40 commits into from Dec 16, 2021
Merged

Template DoCommand and friends #9725

merged 40 commits into from Dec 16, 2021

Conversation

michicc
Copy link
Member

@michicc michicc commented Dec 1, 2021

Motivation / Problem

Command handlers are passed their data in bit-stuffed form, and with a fixed amount of bits available. This is a) prone to errors as there is no compile-time check if parameters where stuffed into the right spot, and b) limits extensibility as there's some reservation to blanket increase the parameters for one single thing.

Both problems can be solved if one allows arbitrary, typed parameters for commands using some C++ magic.

Description and Discussion

With this PR, DoCommand and friends are spiced up using some C++17 template magic to achieve variable amounts of typed parameters. Main implementation points:

  • Command parameter types are extracted by using a non-intrusive type-traits class.
  • These types are reflected into templated DoCommand functions to let the command execution machinery expose typed parameters.
  • Network transfer of commands is accomplished by (de-)serialising the parameters from/to an unstructured byte stream. Adding structure (type markers etc) has not benefit IMHO, as network play is revision-locked anyway.
  • Commands can return more than just a CommandCost, making most of the previously used globals obsolete.
    • Direct command execution (i.e. the old DoCommand) directly returns the command proc returns.
    • Indirect execution (i.e. DoCommandP) passes the return values on to the command callbacks.
    • Command callbacks are compile-time optimized by their type as most callbacks just want success/failure and maybe a tile reference. Some callbacks need the command parameters and the extra return values. With judicious use of if constexpr, the compiler can be tasked with generating matching function calls.
    • The command callback used for AI/GS is special as they use one callback for all commands and still want to capture command parameters for validation in a networking context. To allows this use, one possible form of the callback function receives the command parameters and return values in the same serialised format as used in networking.

Some additional random comments:

  • Modern compilers are scarily good at optimising templates. I checked various things on godbolt along the way to make sure the implementation doesn't completely suck.
    • One of the recent test is here: https://godbolt.org/z/ja9Y7ane6. Looking at the resulting assembly, you'll find that everything in the struct Do execution layer is completely eliminated (including e.g. the various tuple gymnastics), leaving just a plain old static function call.
    • While I didn't check the complete PR with everything in godbolt, I did tests with various bits and bobs and generally found the same as in the example above. Compilers are really good with templated code these days.
  • I'm not really happy with the way command callbacks for AI/GS is solved, but I didn't have any better idea.
  • A bit the same is the network command unpacking that essentially uses a 2D table over CMD_* and all defined callback functions populated with template specialisations to simulate runtime template resolving. Not very elegant, but the only thing I came up with.
  • As typedefs don't introduce new types, TileIndex is changed into a struct to be able to reliably used in template specialisation.

Limitations

There are bugs, 100% guaranteed 😄
This changes the on-network command packet, no idea if this necessitates any changes to the GC or the TURN relay.
I might have forgotten to update some documentation somewhere.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@michicc michicc force-pushed the pr/c++_cmd branch 3 times, most recently from f1027b2 to 5540a67 Compare December 1, 2021 22:39
src/command_func.h Outdated Show resolved Hide resolved
@michicc michicc force-pushed the pr/c++_cmd branch 6 times, most recently from 4102e2d to f9a87d6 Compare December 3, 2021 19:33
src/newgrf_station.cpp Outdated Show resolved Hide resolved
@@ -1275,7 +1276,7 @@ void HandleMissingAircraftOrders(Aircraft *v)
const Station *st = GetTargetAirportIfValid(v);
if (st == nullptr) {
Backup<CompanyID> cur_company(_current_company, v->owner, FILE_LINE);
CommandCost ret = DoCommand(DC_EXEC, CMD_SEND_VEHICLE_TO_DEPOT, v->tile, v->index, 0);
CommandCost ret = DoCommand<CMD_SEND_VEHICLE_TO_DEPOT>::Do(DC_EXEC, v->tile, v->index, 0, {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if anything can be done about it since there's still a Command struct, though DoCommand::Do sounds double. Command::Do would look and sound better. Although that change might also affect naming things like DC_EXEC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there can only be one Command struct(tm). OTOH the current Command struct is used in one single place and could be easily renamed.

I wouldn't mind a different name, but I don't know what everybody else thinks about it.

Copy link
Member

@TrueBrain TrueBrain Dec 4, 2021

Choose a reason for hiding this comment

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

I like the idea of Command::Do, feels neat.

On the subject, I do think Command::Post would be a confusing name, but I struggle with finding a good name. From what I remember, Command::Do can be executed inside a command, but Command::Post cannot; the latter should also always be the one to execute when starting a command.

So something like Command::Execute? Or Command::Safe? It is difficult .. but for sure "Post" confuses me, as you think it is the "post" (vs "pre") handling of the command; which it is not (I think :D) :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Post was just something I could come up with that whoever came up with the initial P might have though of.

If we were to rename it, I like Execute more then Safe.

src/command_func.h Outdated Show resolved Hide resolved
src/command_func.h Show resolved Hide resolved
@michicc
Copy link
Member Author

michicc commented Dec 3, 2021

Famous last words: I might have figured MingW64 out. For whatever reason, it fails on linking when casting a function pointer to its own type, but works if I insert a detour to some other function type in between. (Fingers crossed all other compilers stay happy with it).

And, to surface one comment from Rb: Should it be DoCommand<>::Do/Post/... or Command<>::Do/Post/..., i.e. keep the old DoCommand naming or not?

src/command_func.h Outdated Show resolved Hide resolved
src/ai/ai_instance.cpp Show resolved Hide resolved
src/game/game_instance.cpp Show resolved Hide resolved
src/command.cpp Outdated Show resolved Hide resolved
src/misc/endian_buffer.hpp Outdated Show resolved Hide resolved
src/misc_cmd.cpp Outdated Show resolved Hide resolved
src/misc_cmd.cpp Outdated Show resolved Hide resolved
src/misc_cmd.cpp Outdated Show resolved Hide resolved
src/misc_cmd.cpp Outdated Show resolved Hide resolved
src/misc_cmd.cpp Outdated Show resolved Hide resolved
src/misc_cmd.h Show resolved Hide resolved
@michicc
Copy link
Member Author

michicc commented Dec 4, 2021

Renamed to Command<>:: from DoCommand<>::, but still Command<>:Post due to lack of consensus.

src/ai/ai_instance.cpp Show resolved Hide resolved

if (p1 >= MapSize()) return CMD_ERROR;
TileIndex end_tile = p1;
if (end_tile >= MapSize()) return CMD_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the tile checking in the command code (DoCommandHelper::Do) preventing a tile from being larger than the map size? Also holds for CmdRemoveLongRoad.

Copy link
Member Author

Choose a reason for hiding this comment

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

DoCommandHelper::Do only checks the first parameter (i.e. what used to be tile), but not anything that used to be in p1/p2, as that is the current behaviour. I thought about checking all TileIndex parameters, but I'd have to verify if any command does a different TileIndex validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Lets keep those things out of this PR then ;)
I've also had other outlandish(?) ideas. We are passing Vehicle/Town/...IDs and almost always T obj = T::GetIfValid(id); if (obj == nullptr) return CMD_ERROR, so I was thinking about putting that resolving to object (and validation whether nullptr is allowed) into some "annotation" (with C++ templating magic), That way most of the common code for resolving and checking (also with enums) can be moved out of the Cmd* functions, making it less likely that mistakes are made with the checks for data that comes from the network.
Though that is far beyond the scope of this PR. However, validating of TileIndex values falls somewhere in the same category.

src/road_gui.cpp Outdated Show resolved Hide resolved
src/roadveh_cmd.h Outdated Show resolved Hide resolved
src/script/api/script_object.cpp Outdated Show resolved Hide resolved
src/vehicle_func.h Show resolved Hide resolved
src/command.cpp Outdated Show resolved Hide resolved
src/command.cpp Outdated Show resolved Hide resolved
src/command.cpp Show resolved Hide resolved
src/command_func.h Show resolved Hide resolved
src/command.cpp Show resolved Hide resolved
src/command_func.h Outdated Show resolved Hide resolved
src/command_func.h Outdated Show resolved Hide resolved
src/command_func.h Outdated Show resolved Hide resolved
src/command_func.h Outdated Show resolved Hide resolved
src/network/network_command.cpp Outdated Show resolved Hide resolved
src/network/network_command.cpp Outdated Show resolved Hide resolved
src/network/network_command.cpp Outdated Show resolved Hide resolved
src/script/api/script_object.hpp Outdated Show resolved Hide resolved
src/script/api/script_object.hpp Outdated Show resolved Hide resolved
… cmd/callback combinations.

If the arguments of the callback proc don't match with the command parameters,
we can't do the proper command execution anyway. As such, don't even generate
an unpack function in the first place, saving a bit of unnecessary code bloat.

Validate on receive that the cmd/callback combination is supported, rejecting
clients that try to send invalid values.
Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

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

Lets hope the guaranteed bugs are found quickly; I can't find any more.

@michicc michicc merged commit afc3d71 into OpenTTD:master Dec 16, 2021
@michicc michicc deleted the pr/c++_cmd branch December 16, 2021 21:28
@ldpl
Copy link
Contributor

ldpl commented Dec 17, 2021

I wanted to wait until I had a chance to review it properly before wringing angry posts but you blink twice and it's already merged.

I still didn't really look much into it so maybe some of it is not that bad or at least salvageable but from what I read in the PR description this is a disaster and should be reverted asap. As it only focuses on code improvements and completely ignores the big picture. Where is any consideration for the actual use cases? Where is any analysis of potential features of the command could use? It's been a constant nuisance for many years, I've lost count of how much stuff could've benefited from a well-thought command system. Highlights, gamescripts, game recording, admin port. And all I see in this PR is just templates are cool, templates go brrrr. Even something as simple as its impact on the bandwidth usage in multiplayer is nowhere to be seen. Or some mention of that admin port shit it does. I get that breaking admin api is already commonplace but what is admin client even gonna do with this "data"? It was already bad enough previously, but at least there was some structure to it.

So yeah, changes like this make me want to just quit doing anything OpenTTD-related. You spend years trying to make the best with what you have and then you have to rewrite half of your code every time some core devs decide it's a good idea to disregard any analysis and foresight and break everything for some little gimmick.

@rubidium42
Copy link
Contributor

I agree it's definitely not trivial to get your head around without (prior) knowledge, and I'm sure there are things in the documentation that can be improved upon to make things clearer.

Having said that, I think that this PR is looking at the big picture of the commands within OpenTTD. The foremost two/three issues that this PR tackles is having a fixed size of 2x32 bits (+ 1 string + 1 tile index) for commands, therefor untyped parameters and a lack of flexibility with commands.

Lets take CmdPause as an example. Previously it looked like: CommandCost CmdPause(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 p2, const std::string &text) and now it looks like CommandCost CmdPause(DoCommandFlag flags, PauseMode mode, bool pause). So, the tile and the text have gone and p1 and p2 have become typed. In the network packet you had 3x4 bytes + 1 byte for the text (nul-termination). Now these 13 bytes are replaced by 2 bytes.
For CmdIncreaseLoan this looked like: CommandCost CmdIncreaseLoan(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 p2, const std::string &text) and now becomes CommandCost CmdIncreaseLoan(DoCommandFlag flags, LoanCommand cmd, Money amount). The old CmdIncreaseLoan's comment also mentioned: * @param p2 (bit 2-31) - lower half of amount (lower 2 bits assumed to be 0) so it did some bit stuffing. Why was this done this way in the past? Because of the reservation of adding another 4 bytes (i.e. p3) so we can split the 8-byte Money into two 4 bytes values and have space for the (sub) command. In the new way, we do not perform bitstuffing anymore. We just pass a single Money parameter and everything is handled under water. Now the command parameters have more space, but actually use 4 bytes fewer than before for this command.
I have to confess, that it does not always reduce the number bytes used. Take for example CmdBuildSingleSignal that goes from CommandCost CmdBuildSingleSignal(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 p2, const std::string &text) to CommandCost CmdBuildSingleSignal(DoCommandFlag flags, TileIndex tile, Track track, SignalType sigtype, SignalVariant sigvar, bool convert_signal, bool skip_existing_signals, bool ctrl_pressed, SignalType cycle_start, SignalType cycle_stop, uint8 num_dir_cycle, byte signals_copy). The new variant uses 1 byte more (although with extra logic in the EndianBufferWriter and EndianBufferReader we could reduce the number of bytes by doing bitstuffing under the hood, but that was not scope of this PR).

Due to the "unlimited" amount of parameters you can now pass, there is no need to perform bitstuffing before calling a command and undoing the bitstuffing within the command. You just pass the right values into the parameters. Now, if someone changes the parameters of the function, the compilation for the callers will break instead so you can go to those locations to pass the right information, instead of hoping that you fixed all callers.
Or when we do something like increasing the number of StationClassIDs and we require the type to become larger, we can just make the type larger and all commands will automatically do the right thing. With the old command system we had to look through all the code to see where a StationClassID would be bitstuffed, then we had to change the bitstuffing so the extra byte becomes available. The result: all callers of the commands that use StationClassID need to be identified, and if you have some other code that calls it that is not in main, we would not update it and that command call breaks. With the new code, you would not even notice the StationClassID's size being made larger.

That alone gives a lot more flexibility, but there is more. You are not limited to returning "only" a CommandCost anymore. Take for example CmdCloneVehicle. This was CommandCost CmdCloneVehicle(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 p2, const std::string &text) and became std::tuple<CommandCost, VehicleID> CmdCloneVehicle(DoCommandFlag flags, TileIndex tile, VehicleID veh_id, bool share_orders). With this extra flexibility of returning more than just a CommandCost we can get rid of using a global variable to pass around information between a Command and its callback function.

I do agree that the command logging for the admin port could use some love. We have to check what to do there, but it was already a random (meaningless) blob of data that could be changed at any time. I wonder whether we might now be able to at least give some meaning to the packet; maybe the actual name of the command, and per parameter the type and value in some more useful format. Then an user of the admin logging can at least gain some more knowledge out of the data which would be more stable than it was before.

@ldpl
Copy link
Contributor

ldpl commented Dec 17, 2021

Well, ok, having more data is useful in the long run, so counts as a part of "big picture" I guess. And I never doubted this change is good for refactoring the current code. But at this point, I'd expect any rewrite of the command system to at least achieve that much. What I'm more concerned about is that the way it's done here specifically hinders two of the most essential (and I'll elaborate on it more later) imo properties for the further development of the game itself and supporting code that works with it: the ability to store and analyze commands. There were already some examples for the command storage in the code previously but I guess they weren't significant enough, so here are some potential features that could've used it:

  1. Complex building tool highlights. Every building action requires some kind of preview/indication before the command can be actually executed, so that current action needs to be stored somehow. Right now previews are very simple and are handled by HighLightStyle and TileHighlightData. And while those are already a mess if you ever want a wsywg preview like cmclient does you need to store even more. If you look at cmclient there is a ridiculous construct of HighLightStyle->citymania::ObjectHighlight->CommandContainer->citymania::ObjectTileHighlight.
    But all those user actions have a matching command so if commands can be stored there is no need for a duplicating structure and it can be directly simplified to CommandContainer->citymania::ObjectTileHighlight. With an added benefit of command being able to report it's detailed actions if needed (see (2)).
    Screenshot from 2021-02-17 12-48-18

  2. It's a must for good gameplay with server/gamespript to analyze and interfere with the command flow. A classic example would be stopping other companies from destroying your claimed town in citybuilder. And while having some kind of callback system could be a better approach to this than analyzing command packets it doesn't seem to be possible to ever get callbacks in gamescripts, let alone admin clients. So analyzing command structure or packet seems to be the only option for them to get any information on user actions, especially before their execution.

  3. Command logging for either debug, game recording and/or server moderation purposes. Game recording/replay is probably not affected by this change but debug and moderation are as they require admin and/or developer to read the command output and having just a data blob there isn't helping the cause. Though it looks like this can be solved relatively easily by sending some kind of command schema along with the command names in admin api. This should also cover other cases that require some analytics on command internals. Like, while you already explained how commands are packed I still have no idea how would that impact the bandwith of a regular game and the size of game recording. If I had a schema I could easily calculate that by using the recordings of old games.

  4. Ability to use arbitrary command callbacks. There was (and afaict still is) no way to reliably identify a specific command after it's made the round trip over the network. So any callbacks are identified by a pre-defined table and are validated to match it. And not only it is a big issue for any network-compatible patch like cmclient/cmserver that can't change callback table but it also prevents gamescripts from sending more than one command per tick (or maybe even 2 ticks, don't remember). And that's a huge handicap for gamescripts. For example, updating goals and story book for 15 companies can take several minutes.

Also, I realize that a single PR doesn't have to necessarily solve all the issues but it would be nice to see an acknowledgment of their existence and some plan or thoughts for solutions. Just so that it wouldn't require 4 more rewrites of the command system that only core devs could realistically implement.

J0anJosep added a commit to J0anJosep/OpenTTD that referenced this pull request Dec 18, 2021
J0anJosep added a commit to J0anJosep/OpenTTD that referenced this pull request Dec 18, 2021
@nielsmh
Copy link
Contributor

nielsmh commented Dec 19, 2021

@ldpl Regarding your final point, the storybook and goal commands in specific would probably now benefit from a big pass to perhaps change them fundamentally. For example, allow them to send bulk updates so a single command could create an entire storybook, or update all the goals for a company. That kind of change would be out of scope for this PR, but I'm quite sure it would be worth the work to do.

@ldpl
Copy link
Contributor

ldpl commented Dec 19, 2021

@nielsmh Technically there is not much difference between sending a list of changes in one command and sending a list of commands with changes. Except you'll need to add complex structures to DoCommand templates, implement new GS api methods, etc. And, tbh, no matter how you wrap it up updating UI from the server side is going to suck no matter what. So this overcomplication will ultimately be for nothing. Also, it's not like UI is the only thing that can benefit from the command spam, it's just the most obvious one.

@nielsmh
Copy link
Contributor

nielsmh commented Dec 19, 2021

You mention the update rate as a specific problem, due to the current limit of one simple update per command. If the updates could be batched into a single command, the updates that currently may take several real-time minutes could perhaps be reduced to take 32 game ticks.

@ldpl
Copy link
Contributor

ldpl commented Dec 19, 2021

You'll have to do batching for each command for every specific usecase instead of solvng the issue once and for all. Slow story book - batch story book, slow goals - batch goals, slow town updates - batch town text, batch industry text, batch object placement, etc, etc, etc. Every time someone comes up with something new you'll have to make a batched command.

@michicc
Copy link
Member Author

michicc commented Dec 19, 2021

I can see why batching would make sense, but what does it have to do with how commands are handled in the source code?

Currently scripts are suspended after each issued command because commands are delayed at least one tick in networked games. So the problem with any kind of command batching isn't any templated code, but that you'd have to completely redesign the script API to make command execution asynchronous from the scripts POV. This is a problem of a totally different layer than the command layer changed by this PR.

@ldpl
Copy link
Contributor

ldpl commented Dec 19, 2021

@michicc yes, that problem is of a wider scope than just command layer, but it's related to it nonetheless. If there is no way to identify commands on the network it will be impossible to do asynchronous execution no matter what you do to the script api. So it's something that, at the very least, should be considered when designing the command layer, otherwise you may end up in a situation where for someone to make a progress on that script api redesign he'll have to completely redo your command layer first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants