Why p1 -> end_tile? Don't we always use the p1, p2 signature for all Cmd functions?

I would think it is better to rename the shadow p1 instead?

Similar to the start_tile change btw.

Edit: some quick grepping in the source code, indeed, we always call it p1, p2. We mostly call it tile, with some exceptions. Maybe better to stick to that?

Yes, p1 and p2 are often used for the Cmd functions. However, the parameter names are not part of the function declaration. Code wise it would, in my opinion, be better to have variable/parameter names that tell me something useful than that I would have to look it up. In this case it creates two extra variables just to store tile/p1 with a better name. By the way, looking at the function declaration now... it's not even a real command. Just a helper for a command so renaming p1 is technically less of an issue here I'd guess, and probably text can even be removed.

In other cases there are constructs like TileArea(tile, p2). What is the meaning of p2 there? An encoded width and height? Or a tile? And when tile is defined as the end tile of area-drag why is it the first parameter? So tile is the end... that's unexpected! In my opinion TileArea(start_tile, end_tile) looks way more descriptive, and start_tile is more descriptive than p2.

I can as easily change it back and rename the p1 later down in this code to something else. Though with the other cases I would need to figure out another name for the tile that is used to iterate over tiles in the TileArea. Could of course use t, but that's not really descriptive on its own.

So, the main question is... what's the consensus? Keep tile and p1/p2 as required parameter names for commands? Or allow changing the parameter names to better suit the content.

Keep as p1 & p2, IMO. doxygen comment is supposed to properly state what points to what, and ideally it would be done in the first few lines of the function anyway.

Ideally, It would just be two (or just one?) bitfield that magically separated the types/variables out as appropriate before it even got to the Cmd function - I think @nielsmh had some ideas about that?

The current consensus clearly is p1, p2, as a grep shows :) (140+ use it, only a handful don't). Personally I like that, as it makes it very easy to spot where the information comes from.

The idea behind it was that p1/p2 are always first cast to another variable, to assign it meaning. For example, in this case, you don't cast it to TileIndex. That is not wrong, as it is an uint32, just less verbose.

Either way, I think this is a conversation on a whole different level, and I do not think we should "sneak" that in a PR like this :) If we want to let go of p1, p2 we might want to do it in another PR and going through all 140-ish commands. That also makes it a lot easier for future-us to get what went on, instead of a commit that is seemly unrelated :)

Changed it back, and renamed the other uses of tile to current_tile so it does not overlap with tile from the parameters. I could not use t in some places as there t already referred to Town.