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

Fix: Don't send unused tile field over the network #10507

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

ldpl
Copy link
Contributor

@ldpl ldpl commented Feb 24, 2023

Motivation / Problem

CommandPacket contains a TileIndex field that is rarely used because it's either 0 or duplicates the first tile argument that is sent separately. Also tile can be passed to any command, even if it doesn't have a tile argument and has no use for it or, on contrary, it can be ommited when it's actually used. This makes it harder to understand code and simpler to make mistakes.

Description

This PR removes the tile field from CommandPacket and deals with the command handling according to command type:

  • Has first TileIndex argument. cp->tile just duplicates the argument so no additional handling is needed.
  • Has implicit TileIndex argument. Mark these commands with the new CMD_LOCATION flag to make it more explicit. That makes sure tile is always sent as the first argument in the packet and Post calls so it can't be missed (like in CcBuildWagon). Tile is mainly used for the error location and is handled internally, so it's not included in command handler or Do function arguments.
  • Has no tile argument. Just remove unused cp->tile and make sure tile can't be passed by accident (like in OnDragDrop_Vehicle).

Limitations

This PR focuses on network side of things and doesn't go further in solving TileIndex confusion like

  • Using both 0 and INVALID_TILE
  • Implicit validation of first (and only first) TileIndex argument.
  • Implicit error location handling.
  • It's mostly unnecessary to pass location over the network just to show the error.

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

src/command_func.h Outdated Show resolved Hide resolved
src/command_func.h Outdated Show resolved Hide resolved
@glx22 glx22 merged commit c73b88d into OpenTTD:master Feb 24, 2023
@glx22 glx22 added the backport requested This PR should be backport to current release (RC / stable) label Mar 27, 2023
rubidium42 pushed a commit to rubidium42/OpenTTD that referenced this pull request Mar 28, 2023
rubidium42 pushed a commit to rubidium42/OpenTTD that referenced this pull request Mar 28, 2023
@ldpl ldpl deleted the fix-double-tile branch April 5, 2023 15:11
@rubidium42 rubidium42 added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants