Template DoCommand and friends #9725

michicc · 2021-12-01T22:11:00Z

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')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

src/command_func.h

src/newgrf_station.cpp

rubidium42 · 2021-12-03T21:54:44Z

src/aircraft_cmd.cpp

@@ -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, {});


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.

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.

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

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

michicc · 2021-12-03T22:50:15Z

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

src/ai/ai_instance.cpp

src/game/game_instance.cpp

src/command.cpp

src/misc/endian_buffer.hpp

src/misc_cmd.cpp

src/misc_cmd.h

michicc · 2021-12-04T23:33:40Z

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

src/ai/ai_instance.cpp

rubidium42 · 2021-12-05T08:14:48Z

src/road_cmd.cpp

-
-	if (p1 >= MapSize()) return CMD_ERROR;
-	TileIndex end_tile = p1;
+	if (end_tile >= MapSize()) return CMD_ERROR;


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.

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.

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

src/roadveh_cmd.h

src/script/api/script_object.cpp

src/vehicle_func.h

src/command.cpp

src/command_func.h

src/command.cpp

src/command_func.h

src/network/network_command.cpp

src/script/api/script_object.hpp

…ion call.

…nd function calls.

…nd*.

…templated command traits. This is using a non-intrusive type-traits like templated system, which allows compile-time validation that the command table and the command enum match up.

…dlers.

…cked arguments.

…ept Script).

…return values.

…procs.

… 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.

rubidium42

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

ldpl · 2021-12-17T15:06:20Z

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 · 2021-12-17T19:28:21Z

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 · 2021-12-17T22:44:59Z

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:

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)).
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.
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.
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.

nielsmh · 2021-12-19T10:34:58Z

@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 · 2021-12-19T10:44:06Z

@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 · 2021-12-19T10:46:28Z

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 · 2021-12-19T10:49:54Z

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 · 2021-12-19T11:06:32Z

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 · 2021-12-19T11:17:41Z

@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.

… cmd_helper.h.

…lper.h.

michicc force-pushed the pr/c++_cmd branch 3 times, most recently from f1027b2 to 5540a67 Compare December 1, 2021 22:39

frosch123 reviewed Dec 1, 2021

View reviewed changes

src/command_func.h Outdated Show resolved Hide resolved

michicc force-pushed the pr/c++_cmd branch 6 times, most recently from 4102e2d to f9a87d6 Compare December 3, 2021 19:33

rubidium42 reviewed Dec 3, 2021

View reviewed changes

michicc force-pushed the pr/c++_cmd branch from f9a87d6 to da67bfd Compare December 3, 2021 22:50

TrueBrain reviewed Dec 4, 2021

View reviewed changes

src/command_func.h Outdated Show resolved Hide resolved

rubidium42 reviewed Dec 4, 2021

View reviewed changes

michicc force-pushed the pr/c++_cmd branch from da67bfd to 9064a1c Compare December 4, 2021 23:33

rubidium42 reviewed Dec 5, 2021

View reviewed changes

michicc force-pushed the pr/c++_cmd branch from 9064a1c to 99d76b3 Compare December 5, 2021 13:07

rubidium42 mentioned this pull request Dec 5, 2021

Codechange: do not use all upper case enumerators in a scoped enum #9733

Merged

rubidium42 reviewed Dec 14, 2021

View reviewed changes

michicc added 9 commits December 15, 2021 01:00

Fix: Template syntax error when using 'span' with a container type.

d5f67af

Codechange: Move command arguments to the back of the DoCommand funct…

d7c3d1e

…ion call.

Codechange: Move command arguments to the back of the networked comma…

b3eec5f

…nd function calls.

Codechange: Untangle command code, flags and error string for DoComma…

def6df7

…nd*.

Codechange: Declare our custom enum operators as constexpr.

9824b93

Codechange: Let the compile generate the master command table out of …

1ec5d9d

…templated command traits. This is using a non-intrusive type-traits like templated system, which allows compile-time validation that the command table and the command enum match up.

Codechange: Move flags in CommandProc in front of the command arguments.

7ba2c8f

Codechange: Move command callback declarations to the cmd header files.

d2d955d

Codechange: Align parameter order of command callbacks to command han…

c7d49a2

…dlers.

michicc added 8 commits December 15, 2021 01:00

Codechange: Template the command callback function type to allow unpa…

679ac34

…cked arguments.

Codechange: Pass unpacked command arguments to command callbacks (exc…

2fb26ce

…ept Script).

Codechange: Add support for additional command result values.

bd4c76f

Codechange: Don't use globals for story/goal/sign/group command proc …

267df5f

…return values.

Codechange: Don't use globals for return values from vehicle command …

62e9127

…procs.

Codechange: Don't use a global for the 'not enough cash' message.

b6a0aa7

Codechange: Don't use a global for the terrforming error tile.

067efdc

michicc force-pushed the pr/c++_cmd branch from c0d566e to f9a7b3f Compare December 15, 2021 00:00

rubidium42 approved these changes Dec 16, 2021

View reviewed changes

michicc merged commit afc3d71 into OpenTTD:master Dec 16, 2021

michicc deleted the pr/c++_cmd branch December 16, 2021 21:28

J0anJosep added a commit to J0anJosep/OpenTTD that referenced this pull request Dec 18, 2021

Cleanup OpenTTD#9725: Cleanup doc and fix typo.

aab46bd

J0anJosep added a commit to J0anJosep/OpenTTD that referenced this pull request Dec 18, 2021

Cleanup OpenTTD#9725: Clean up doc and fix typo.

87bec0d

michicc pushed a commit that referenced this pull request Dec 19, 2021

Cleanup #9725: Clean up Doxygen docs and fix typo. (#9753)

e890e7d

rubidium42 mentioned this pull request Dec 20, 2021

Doc: Fix some documentation typos #9586

Closed

J0anJosep added a commit to J0anJosep/OpenTTD that referenced this pull request Jan 8, 2022

Cleanup OpenTTD#9725: Replace cmd_helper related functions and remove…

449ea57

… cmd_helper.h.

J0anJosep mentioned this pull request Jan 8, 2022

Cleanup #9725: Replace cmd_helper related functions and remove cmd_helper.h #9783

Merged

michicc pushed a commit that referenced this pull request Feb 2, 2022

Cleanup #9725: Replace cmd_helper related functions and remove cmd_he…

d9a37c9

…lper.h.

ldpl mentioned this pull request Aug 30, 2022

Cleanup: Remove duplicate command_type.h include #9986

Merged

ldpl mentioned this pull request Mar 3, 2023

Fix: Restore using founder client name as company manager name #10535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template DoCommand and friends #9725

Template DoCommand and friends #9725

michicc commented Dec 1, 2021 •

edited

Loading

rubidium42 Dec 3, 2021

michicc Dec 3, 2021

TrueBrain Dec 4, 2021 •

edited

Loading

michicc Dec 4, 2021

michicc commented Dec 3, 2021

michicc commented Dec 4, 2021

rubidium42 Dec 5, 2021

michicc Dec 5, 2021

rubidium42 Dec 5, 2021

rubidium42 left a comment

ldpl commented Dec 17, 2021 •

edited

Loading

rubidium42 commented Dec 17, 2021

ldpl commented Dec 17, 2021 •

edited

Loading

nielsmh commented Dec 19, 2021

ldpl commented Dec 19, 2021

nielsmh commented Dec 19, 2021

ldpl commented Dec 19, 2021 •

edited

Loading

michicc commented Dec 19, 2021

ldpl commented Dec 19, 2021

Template DoCommand and friends #9725

Template DoCommand and friends #9725

Conversation

michicc commented Dec 1, 2021 • edited Loading

Motivation / Problem

Description and Discussion

Limitations

Checklist for review

rubidium42 Dec 3, 2021

Choose a reason for hiding this comment

michicc Dec 3, 2021

Choose a reason for hiding this comment

TrueBrain Dec 4, 2021 • edited Loading

Choose a reason for hiding this comment

michicc Dec 4, 2021

Choose a reason for hiding this comment

michicc commented Dec 3, 2021

michicc commented Dec 4, 2021

rubidium42 Dec 5, 2021

Choose a reason for hiding this comment

michicc Dec 5, 2021

Choose a reason for hiding this comment

rubidium42 Dec 5, 2021

Choose a reason for hiding this comment

rubidium42 left a comment

Choose a reason for hiding this comment

ldpl commented Dec 17, 2021 • edited Loading

rubidium42 commented Dec 17, 2021

ldpl commented Dec 17, 2021 • edited Loading

nielsmh commented Dec 19, 2021

ldpl commented Dec 19, 2021

nielsmh commented Dec 19, 2021

ldpl commented Dec 19, 2021 • edited Loading

michicc commented Dec 19, 2021

ldpl commented Dec 19, 2021

michicc commented Dec 1, 2021 •

edited

Loading

TrueBrain Dec 4, 2021 •

edited

Loading

ldpl commented Dec 17, 2021 •

edited

Loading

ldpl commented Dec 17, 2021 •

edited

Loading

ldpl commented Dec 19, 2021 •

edited

Loading