Conversation
SM internally maintained both a case-sensitive and a case-insensitive lookup method for commands, where the case-sensitive hashmap was used as a fast path, and case-insensitive iteration over a list used as the slow path if a command was not found in the hashmap. But only command dispatch handling used this dual path approach, chat triggers for example only did a loopup in the hashmap. Over the years Valve has made more and more of the command dispatch logic case-insensitive to the point where all console commands are now case-insensitive, so maintaining case sensitivity when using chat triggers does not make a lot of sense. There are somewhat popular plugins that attempt to "correct" this behaviour - but at least one is having issues after the previous case-sensitivity fixes for commands - see #1480. We still have to keep the list around for the sorted help use case and command iteration, but this PR changes the hashmap to use a case-insensitive hashing policy (as previously done for convars, and more recently for game command lookup) and changes all by-name lookup to exclusively use the hashmap (as there is no need to fall back to the list any more). Tested a bunch in TF2, I don't know of any games that still have a case-sensitive command dispatch pipeline to test. I think the worst case would be that we'd accept a chat command in the "wrong" case then fail to execute the underlying command. If that turns out to be an issue in practice, we should be able to fix it easily enough by replacing the command name in the buffer with the correct casing of the command we looked up. Also fixed a couple of very minor Lookup vs. Key issues in NameHashSet (noted in #1529) that were being masked due to CharsAndLength's converting constructor. I tried to make the constructor explicit to avoid this happening in the future but HashTable's add function relies on being able to do an implicit conversion so that wasn't possible. We might want to just rely on the implicit conversion up here as well, but it doesn't really matter either way. Fixes #1480, #1529
dvander
approved these changes
Jul 18, 2021
Member
dvander
left a comment
There was a problem hiding this comment.
Weird, why is HashTable trying to convert to a CharsAndLength?
Member
Author
It's something to do with the key type being std::string and the lookup type being CharsAndLength, the add function that takes a key needs to be able to convert it to the lookup type to call findForAdd, but also needs it in key form later to do the actual insert. |
Member
|
Weird, sounds like a design flaw or bug in how it uses the policy. The intent was to have lookup types separate from the key, to avoid unnecessary allocations. But it's probably just over-complicated in any situation SM uses hashtables. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SM internally maintained both a case-sensitive and a case-insensitive
lookup method for commands, where the case-sensitive hashmap was used as
a fast path, and case-insensitive iteration over a list used as the slow
path if a command was not found in the hashmap. But only command
dispatch handling used this dual path approach, chat triggers for
example only did a loopup in the hashmap.
Over the years Valve has made more and more of the command dispatch
logic case-insensitive to the point where all console commands are now
case-insensitive, so maintaining case sensitivity when using chat
triggers does not make a lot of sense. There are somewhat popular
plugins that attempt to "correct" this behaviour - but at least one is
having issues after the previous case-sensitivity fixes for commands -
see #1480.
We still have to keep the list around for the sorted help use case and
command iteration, but this PR changes the hashmap to use a
case-insensitive hashing policy (as previously done for convars, and
more recently for game command lookup) and changes all by-name lookup to
exclusively use the hashmap (as there is no need to fall back to the
list any more).
Tested a bunch in TF2, I don't know of any games that still have a
case-sensitive command dispatch pipeline to test. I think the worst case
would be that we'd accept a chat command in the "wrong" case then fail
to execute the underlying command. If that turns out to be an issue in
practice, we should be able to fix it easily enough by replacing the
command name in the buffer with the correct casing of the command we
looked up.
Also fixed a couple of very minor Lookup vs. Key issues in NameHashSet
(noted in #1529) that were being masked due to CharsAndLength's
converting constructor. I tried to make the constructor explicit to
avoid this happening in the future but HashTable's add function relies
on being able to do an implicit conversion so that wasn't possible. We
might want to just rely on the implicit conversion up here as well, but
it doesn't really matter either way.
Fixes #1480, #1529