Skip to content

refactor(commandxlat): Split evaluateContextCommand into per-command handlers#3008

Open
bobtista wants to merge 3 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/refactor/evaluatecontextcommand-phase1
Open

refactor(commandxlat): Split evaluateContextCommand into per-command handlers#3008
bobtista wants to merge 3 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/refactor/evaluatecontextcommand-phase1

Conversation

@bobtista

@bobtista bobtista commented Jul 23, 2026

Copy link
Copy Markdown

Replaces #2714 by @RikuAnt (tracking #1192), rebased onto current main.

evaluateContextCommand was ~830 lines. This splits it into per-command handlers, leaving the function as the dispatch chain alone (~180 lines), with no change to behavior:

  • resolveContextTarget / resolveGuiCommandTarget — null out draw/obj so evaluation falls back to a position interaction (masked, mine, force-move/attack; shrubbery/mine/relationship filtering).
  • 21 handleXCommand helpers — one per arm of the else-if chain.

Since #2765 (merge ZH CommandXlat) and #2805 (move CommandXlat to Core), this lives in Core/, which is what #2714 was blocked on.

Notes

  • Handler bodies are statement-for-statement moves of the arms they replace, verified mechanically against the pre-refactor revision.
  • Two exceptions, both merges of arms that were identical apart from one constant: handleAttackObjectCommand takes the hint type (ATTACKRESULT_POSSIBLE / ATTACKRESULT_POSSIBLE_AFTER_MOVING), and handleHackCommand takes the special power (disable vehicle / steal cash / disable building).
  • handleSetRallyPointCommand and handleImpossibleAttackHint drop a dead msgType = MSG_INVALID that was immediately overwritten.
  • The result assignment stays inside the ATTACKRESULT_POSSIBLE condition — two later arms read it, so hoisting it into a helper would have changed behavior.
  • Legacy space/mixed indentation in the moved bodies is normalized to tabs; without it those lines land at column 0 once the enclosing scopes are gone.
  • Incorporates the refactor: Refactor evaluateContextCommand phase 1 (#1192) #2714 feedback: verbose refactor comments removed, mixed tab/space indentation fixed, braces on their own lines.

@bobtista bobtista self-assigned this Jul 23, 2026
@bobtista bobtista added the Refactor Edits the code with insignificant behavior changes, is never user facing label Jul 23, 2026
@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown

Greptile Summary

Refactors context-command evaluation without changing its intended behavior.

  • Extracts target normalization, GUI-command handling, waypoint handling, and individual action dispatch paths into private helpers.
  • Replaces the nested action guard and dispatch chain with early returns.
  • Adds the corresponding helper declarations to CommandXlat.h.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failures remain.

Important Files Changed

Filename Overview
Core/GameEngine/Include/GameClient/CommandXlat.h Declares the private helper methods used to decompose context-command evaluation.
Core/GameEngine/Source/GameClient/MessageStream/CommandXlat.cpp Extracts the existing target normalization and ordered command-dispatch logic into helpers while preserving branch order and outcomes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[evaluateContextCommand] --> B[Resolve context target]
    B --> C{Selection or action guard exits?}
    C -->|Yes| D[Return message type]
    C -->|No| E{Waypoint mode?}
    E -->|Yes| F[Handle waypoint command]
    E -->|No| G{GUI command active?}
    G -->|Yes| H[Normalize GUI target and dispatch]
    G -->|No| I[Evaluate ordered context actions]
    I --> J[Delegate to matching helper]
    I --> K[Handle default movement]
    F --> D
    H --> D
    J --> D
    K --> D
Loading

Reviews (2): Last reviewed commit: "refactor(commandxlat): Split evaluateCon..." | Re-trigger Greptile

@xezon xezon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this refactor does not go far enough. Function can be split into many small handle functions.

GameMessage::Type CommandTranslator::evaluateContextCommand( Drawable *draw,
const Coord3D *pos,
CommandEvaluateType type )
void CommandTranslator::normalizeContextInputs(Drawable*& draw, Object*& obj, Drawable*& drawableInWay)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is normalizeContextInputs a fitting name for this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

no, renamed to resolveContextTarget. Same for normalizeGuiCommandTarget while I'm at it.

@bobtista bobtista changed the title refactor(commandxlat): Extract helpers from evaluateContextCommand refactor(commandxlat): Split evaluateContextCommand into per-command handlers Jul 25, 2026
@bobtista

Copy link
Copy Markdown
Author

I think this refactor does not go far enough. Function can be split into many small handle functions.

Agreed. I took another pass and split evaluateContextCommand into 21 per-command handlers, plus separate target-resolution, GUI-command, waypoint, and default-move helpers. The main function is now only the ordered dispatch chain, with its behavior and branch order preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Edits the code with insignificant behavior changes, is never user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants