Skip to content

Rebuild the hotkey lookup whenever bindings change #3082

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.

Sign up for GitHub

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

Open
burnhamrobertp wants to merge 1 commit into
base: master
Choose a base branch
from
+34 −3
Open

Rebuild the hotkey lookup whenever bindings change #3082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions rts/Game/UI/KeyBindings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ void CKeyBindings::Init()
keyChainTimeout = 750;

buildHotkeyMap = true;
hotkeysDirty = false;
debugEnabled = false;

codeBindings.reserve(32);
Expand Down Expand Up @@ -611,6 +612,7 @@ void CKeyBindings::AddActionToKeyMap(KeyMap& bindings, Action& action)
ActionList& al = bindings[ks];
action.bindingIndex = ++bindingsCount;
al.push_back(action);
hotkeysDirty = true;
} else {
ActionList& al = it->second;
assert(it->first == ks);
Expand All @@ -624,6 +626,7 @@ void CKeyBindings::AddActionToKeyMap(KeyMap& bindings, Action& action)
// not yet bound, push it
action.bindingIndex = ++bindingsCount;
al.push_back(action);
hotkeysDirty = true;
}
}
}
Expand Down Expand Up @@ -683,6 +686,9 @@ bool CKeyBindings::UnBind(const std::string& keystr, const std::string& command)
if (al.empty())
bindings.erase(it);

if (success)
hotkeysDirty = true;

return success;
}

Expand All @@ -707,6 +713,7 @@ bool CKeyBindings::UnBindKeyset(const std::string& keystr)
return false;

bindings.erase(it);
hotkeysDirty = true;
return true;
}

Expand Down Expand Up @@ -740,7 +747,12 @@ bool CKeyBindings::UnBindAction(const std::string& command)
RECOIL_DETAILED_TRACY_ZONE;
if (debugEnabled)
LOG("[CKeyBindings::%s] command=%s", __func__, command.c_str());
return RemoveActionFromKeyMap(command, codeBindings) || RemoveActionFromKeyMap(command, scanBindings);
const bool changed = RemoveActionFromKeyMap(command, codeBindings) || RemoveActionFromKeyMap(command, scanBindings);

if (changed)
hotkeysDirty = true;

return changed;
}


Expand Down Expand Up @@ -824,6 +836,7 @@ void CKeyBindings::LoadDefaults()
}

buildHotkeyMap = tmpBuildHotkeyMap;
MaybeRebuildHotkeyMap();
}


Expand Down Expand Up @@ -930,6 +943,7 @@ bool CKeyBindings::ExecuteCommand(const std::string& line)
keyCodes.Reset();
scanCodes.Reset();
bindingsCount = 0;
hotkeysDirty = true;
Bind("enter", "chat"); // bare minimum

if (debugEnabled)
Expand All @@ -939,8 +953,7 @@ bool CKeyBindings::ExecuteCommand(const std::string& line)
return false;
}

if (buildHotkeyMap)
BuildHotkeyMap();
MaybeRebuildHotkeyMap();

return false;
}
Expand Down Expand Up @@ -979,11 +992,25 @@ bool CKeyBindings::Load(const std::string& filename)
loadStack.pop_back();

buildHotkeyMap = tmpBuildHotkeyMap;
MaybeRebuildHotkeyMap();
Comment on lines 994 to +995

@sprunk sprunk Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wouldn't doing the change below (and in the other tmp in line -826/+838) let us avoid introducing the new "dirty" bool and replace it with the existing buildHotkeyMap?

Suggested change
buildHotkeyMap = tmpBuildHotkeyMap;
MaybeRebuildHotkeyMap();
buildHotkeyMap |= tmpBuildHotkeyMap;
MaybeRebuildHotkeyMap();

@burnhamrobertp burnhamrobertp Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not quite - that line wouldn't do anything on its own. buildHotkeyMap is already false at that point (Load sets it false for the whole batch), so |= and = end up the same there.

To actually drop the extra flag and reuse buildHotkeyMap, Bind/UnBind would have to set buildHotkeyMap true whenever they change a binding. The catch is that Load runs the keys file one line at a time through ExecuteCommand, and the MaybeRebuildHotkeyMap call at the bottom of ExecuteCommand runs for every one of those lines. So the moment a line binds something and sets the flag, that same line rebuilds the whole map, and you end up rebuilding once per binding in the file instead of once at the end. Setting buildHotkeyMap false during Load is what avoids that today.

So I think it needs to stay two separate things: buildHotkeyMap for "are we in the middle of a load" and hotkeysDirty for "did a binding actually change". That's what lets it skip rebuilding on every line and just rebuild once when the load finishes.

@burnhamrobertp burnhamrobertp Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Going incremental would sidestep this and simplify things - each bind/unbind would keep its own entry current and the rebuild flags could go away. I don't think it's worth it here though: keybindings get read constantly (icon labels, GetActionHotKeys) but changed rarely, so rebuilding the whole map from the current bindings when something changes stays simple and can't leave it half-updated or out of sync with the real bindings, since it's always derived fresh. Incremental would spread that bookkeeping across every path that changes a binding, which is more room for exactly the stale-map problem this fixes. So I'd rather keep the full rebuild and just make sure it runs at the right times, which is what this does.

@sprunk sprunk Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was thinking something like this master...sprunk-patch-11

where there's only calls to MaybeRebuild at the end of every public function, and nothing calls those public functions internally (instead everything calls an Internal equivalent that looks the same and just doesn't have MaybeRebuild). So evey possible call path from the outside has exactly one call to MaybeRebuild at the end. Then you can have just one bool, everything sets it when anything changes, and then it's only checked once at the end.

@burnhamrobertp burnhamrobertp Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah I see now, yeah I don't see any reason why this wouldn't work, and its cleaner from an interface perspective, relies less on convention (I've not looked around but I assume the notion of having methods suffixed with Internal occurs elsewhere)


return true;
}


void CKeyBindings::MaybeRebuildHotkeyMap()
{
RECOIL_DETAILED_TRACY_ZONE;
// buildHotkeyMap is false only while suppressed inside a batch load, so the
// outermost op rebuilds once; hotkeysDirty skips ops that changed nothing.
if (!buildHotkeyMap || !hotkeysDirty)
return;

BuildHotkeyMap();
hotkeysDirty = false;
}


void CKeyBindings::BuildHotkeyMap()
{
RECOIL_DETAILED_TRACY_ZONE;
Expand Down
4 changes: 4 additions & 0 deletions rts/Game/UI/KeyBindings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ class CKeyBindings : public CommandReceiver

protected:
void BuildHotkeyMap();
// Rebuilds the reverse cache once per outermost op, and only if bindings
// actually changed (buildHotkeyMap gates batch nesting, hotkeysDirty the change).
void MaybeRebuildHotkeyMap();
void DebugActionList(const ActionList& actionList) const;

void AddActionToKeyMap(KeyMap& bindings, Action& action);
Expand Down Expand Up @@ -91,6 +94,7 @@ class CKeyBindings : public CommandReceiver
int keyChainTimeout = 750;

bool buildHotkeyMap = true;
bool hotkeysDirty = false;
bool debugEnabled = false;
};

Expand Down
Loading