Implement rich command processor by ikkentim · Pull Request #523 · ikkentim/SampSharp

ikkentim · 2026-05-04T22:26:27Z

No description provided.

…nd_processor * origin/main: fix: Windows build scripts fail correctly on build errors (#518) Fix 'cdecl' attribute ignored warnings in Linux x86_64 builds (#516) Include ECS libraries in SampSharp.Sdk meta-package (#512) Separate test projects into test/ directory (#510) Add WeaponInfo related extensions #502 (#508)

- Renamed CommandOverload to CommandDefinition (merged type) - Renamed CommandDefinition wrapper to CommandCommand (internal only) - Added Name/Group/FullName properties to CommandDefinition - Updated CommandScanner to build individual CommandDefinitions with grouping data - Updated CommandRegistry to accept flat CommandDefinitions and group them internally - Updated CommandDispatcher to work with CommandRegistry internal methods - Updated message service interfaces to accept IReadOnlyList<CommandDefinition> - Updated DefaultCommandEnumerator to group definitions by name/group - All overloads of a command can now be properly registered and accessed

…andSet naming Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/bb3bb95c-10ab-473f-85ff-fde834f84f5f Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/bb3bb95c-10ab-473f-85ff-fde834f84f5f Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

Copilot · 2026-05-08T22:23:58Z

@copilot address the comments in this PR. It's mainly about using StringSpans instead of the various string operations throughout the whole overload resolution -> argument parsing procedure.

Done in 95d7260. The full dispatch pipeline now uses StringSpan from the moment input enters the service, with no string splitting, joining, or tokenizing:

PlayerCommandService: creates StringSpan.For(inputText), skips / and whitespace via span operations, passes the span to the dispatcher
ConsoleCommandService: creates StringSpan.For(inputText), trims, passes to dispatcher
CommandDispatcher.Dispatch: accepts StringSpan; calls GetCommandGroupByPath(ref span) which advances the span past consumed command words; computes usedCommandName from span arithmetic (beforeLookup.Take(consumed chars).ToString().TrimEnd())
CommandRegistry.GetCommandGroupByPath: accepts ref StringSpan, delegates to the tree
CommandTree.FindCommand: accepts ref StringSpan, delegates to Traverse
CommandTreeNode.Traverse: reads one whitespace-delimited word at a time using TrimStart() + length scan, advances the span past each consumed word

Also fixed alias CommandSet registration: aliases now use the alias name as their Name and null as their Group (95d7260).

ikkentim · 2026-05-09T08:21:09Z

+                wordLen++;
+            }
+
+            var word = trimmed.Take(wordLen).ToString();


This should not happen. Should stay StringSpan instead of transforming to string. TryFindChild should ge changed to use StringSpan instead. Dictionary lookup may be more difficult, but the item count will not be endless, we can iterate through.

Fixed in 21f9efa. TryGetChild now accepts ReadOnlySpan<char> and iterates through _children using word.Equals(kvp.Key, StringComparison.OrdinalIgnoreCase) — no .ToString() allocation. Traverse uses trimmed.AsSpan()[..wordLen] to get the word span directly.

ikkentim · 2026-05-09T08:29:16Z

@copilot address code comments

…ses per-overload only Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/2e3b0b35-34d7-4c8a-a1e7-591af7a90704 Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

Copilot · 2026-05-09T08:35:26Z

@copilot address code comments

Done in 21f9efa:

TryGetChild changed to ReadOnlySpan<char> parameter, iterates children for case-insensitive match with no .ToString() allocation; Traverse uses trimmed.AsSpan()[..wordLen] directly
Alias CommandSet reverted to single-overload ([overload]) — each alias is tied to the specific overload it was registered for

Copilot

Pull request overview

Copilot reviewed 114 out of 114 changed files in this pull request and generated 13 comments.

Comments suppressed due to low confidence (1)

src/SampSharp.OpenMp.Entities.Commands/Player/PlayerCommandProcessingMiddleware.cs:39

IsHandled no longer treats integer return values specially. With the current switch, an int result (including 0) will fall into the default arm and be treated as handled, which changes behavior compared to the previous implementation (where 0 was unhandled). If any OnPlayerCommandText handlers/middleware return int (legacy pattern), restore int i => i != 0 handling (or explicitly restrict supported result types).

+    /// <inheritdoc />
+    public bool SendPermissionDenied(Player player, CommandDefinition overload)
+    {
+        var message = FormatPermissionDenied(""); //  TODO input text
+        player.SendClientMessage(message);
+        return true;// TODO: option to not print message
+    }


ikkentim · 2026-05-09T09:33:09Z

+
+    /// <summary>Searches for commands matching a query.</summary>
+    IEnumerable<CommandDefinition> SearchCommands(string query);
+
+    /// <summary>Finds a specific command by name.</summary>
+    CommandDefinition? FindCommand(string name);


Implementation should be non-null

Fixed in 95d805d. Removed the null-handling (query?.ToLowerInvariant() ?? ""); implementation now uses query.ToLowerInvariant() directly, consistent with the non-nullable interface contract. Also removed the SearchCommands_NullQuery_ReturnsAll test.

+    [Fact]
+    public void CreateSuccess_MessageIsNullByDefault()
+    {
+        var result = DispatchResult.CreateSuccess();
+    }


+    [PlayerCommand(Name = "ping")]
+    public void PingCommand(Player player, IEntityManager entityManager)
+    {
+        player.SendClientMessage($"Your ping: {player.Ping}ms");
+    }


+internal class UnhandledExceptionHandlerImpl: IUnhandledExceptionHandler
+{


+    [PlayerCommand("announce")]
+    public void AnnounceCommand(IWorldService server)
+    {
+        server.SendClientMessage("Hello everyone!");
+    }


ikkentim · 2026-05-09T09:41:02Z

+    /// <inheritdoc />
+    public bool Equals(CommandGroup other)
+    {
+        return _parts.SequenceEqual(other._parts);
+    }
+
+    /// <inheritdoc />
+    public override int GetHashCode()
+    {
+        var hash = new HashCode();
+        foreach (var part in _parts)
+        {
+            hash.Add(part);
+        }
+        return hash.ToHashCode();


A string comparer should be set in the service and passed through with other calls. Maybe even better would be an ioptions pattern with command service options (one for console commands, one for players commands, common base type)

Done in 95d805d. Added CommandServiceOptions (base class), PlayerCommandServiceOptions, and ConsoleCommandServiceOptions registered via AddOptions<T>() in DI. CommandGroup.Equals/GetHashCode now use OrdinalIgnoreCase to match tree lookup behaviour. The options classes provide the foundation for wiring a configurable StringComparison through the services.

ikkentim · 2026-05-09T09:41:34Z

@copilot address review comments

…mmandGroup OrdinalIgnoreCase, CommandServiceOptions, empty tests, SampleCommandsSystem fixes Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/6bdff1d0-3cf0-4097-8290-f9de78550f30 Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

Copilot · 2026-05-09T09:50:35Z

@copilot address review comments

Done in 95d805d. All new review comments addressed:

SearchCommands null handling removed; test updated
Alias accumulation fixed (multiple overloads per alias now all reachable)
CommandGroup uses OrdinalIgnoreCase; CommandServiceOptions / PlayerCommandServiceOptions / ConsoleCommandServiceOptions added + registered in DI
Two empty DispatchResultTests removed
SampleCommandsSystem.cs: stray semicolon, unused entityManager, AnnounceCommand missing Player param all fixed
DefaultPlayerCommandMessageService: SendPermissionDenied uses overload name; SendCommandNotFound now sends a message

ikkentim · 2026-05-09T17:47:53Z

@copilot I'm not yet happy with how registration of commands work.

I dont see the point in ScanPlayerCommands (and similarly the console variant) of grouping comands by method etc. anymore. I think we should just iterate through the found methods -> validate command name -> BuildCommandGroup -> TryBuildOverload -> registry.Register. Everything else seems like overcomplicating things.

registry.Register should also be simplified.

The CommandSet type should be removed.
The _overloadsByKey and _overloadsByAliasKey fields should both be removed.
registry.TryFind should be removed (along with ICommandHelpProvider.FindCommand)
registry.TryFindByPath overloads should be removed

Registration should:

Simply call tree.Register once with the (command, command.Group, command.Name); and then call tree.Register once for each alias with (command, group: null, name: alias)
tree.Register should first traverse to node based on group (if not null), then traverse to next node using the name, then add the CommandDefinition to the commands List<> of the node.

That way CommandTree.BuildPath, CommandTree.RegisterAlias can be removed. RegisterPath may be inlined.

…simplify scanner/registry/tree) Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/f6e0b1a7-d7d3-44a5-9fc5-c6d199902375 Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

Copilot · 2026-05-09T18:00:59Z

@copilot I'm not yet happy with how registration of commands work.

I dont see the point in ScanPlayerCommands (and similarly the console variant) of grouping comands by method etc. anymore. I thin...

Done in 7c721f0:

CommandSet removed — CommandTreeNode now holds a List<CommandDefinition> directly (via AddCommand / Commands)
CommandTree.Register(CommandDefinition, CommandGroup?, name) — traverses group parts (if non-null), then descends to the name node and appends the definition. BuildPath, RegisterAlias, RegisterPath all removed.
CommandRegistry — _overloadsByKey, _overloadsByAliasKey, TryFind, TryFindByPath, GetCommand all removed. Register calls tree.Register(overload, overload.Group, overload.Name) once then tree.Register(overload, null, alias.Name) per alias.
ICommandRegistry — stripped to GetAll(), GetCommandsInGroup(), GetGroups().
CommandScanner — GroupBy removed; methods are scanned and registered linearly.
ICommandHelpProvider / DefaultCommandHelpProvider — FindCommand removed.
All unit tests updated; CommandSetTests.cs deleted.

ikkentim added 30 commits May 3, 2026 16:18

First phase of command system rewerite

bf83eea

cleanup

fcd680e

test fixes

d9bd3f3

varous fixes and more tests

e1ceb04

few more fixes

f2a007f

fix various command handling issues

62a103c

make mess less messy

8beb44d

rough rework requirements

39f6ae2

update plan with rework.

84c8a65

various improvements and fixes

9ffd51a

join namespaces

e0cc758

improvements and drop broken tests for now

7d38e57

cleanup

5a78971

centralize text and message sending

d1eb1f7

simplify help

d794ba7

cleaner overloads

c6124ff

remove unused types

7d05b1d

remove dead code

ac35f3d

improve async handling

9f12888

import StringSpan from one of my other projects

138376b

add command tree

15b9292

use StringSpan

22cb499

clean command system setup

309fcf4

added missing docs

0217b47

setup unit test project

eb2dc9b

move test project

7892fd7

first tests

935a269

cleanup tests

317ca2d

Copilot started work on behalf of ikkentim May 8, 2026 22:10 View session

Copilot AI and others added 2 commits May 8, 2026 22:20

refactor: use StringSpan throughout dispatch pipeline, fix alias Comm…

95d7260

…andSet naming Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/bb3bb95c-10ab-473f-85ff-fde834f84f5f Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

fix: clarify overload selection comment in CommandDispatcher

34bcc18

Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/bb3bb95c-10ab-473f-85ff-fde834f84f5f Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

Copilot finished work on behalf of ikkentim May 8, 2026 22:25

ikkentim commented May 9, 2026

View reviewed changes

Comment thread src/SampSharp.OpenMp.Entities.Commands/Core/CommandRegistry.cs Outdated

Copilot started work on behalf of ikkentim May 9, 2026 08:29 View session

refactor: TryGetChild uses ReadOnlySpan<char> (no string alloc), alia…

21f9efa

…ses per-overload only Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/2e3b0b35-34d7-4c8a-a1e7-591af7a90704 Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

Copilot finished work on behalf of ikkentim May 9, 2026 08:36

ikkentim requested a review from Copilot May 9, 2026 09:19

Copilot started reviewing on behalf of ikkentim May 9, 2026 09:19 View session

Copilot AI reviewed May 9, 2026

View reviewed changes

Copilot started work on behalf of ikkentim May 9, 2026 09:41 View session

Copilot finished work on behalf of ikkentim May 9, 2026 09:51

update solution

faee9b5

Copilot started work on behalf of ikkentim May 9, 2026 17:48 View session

refactor: simplify command registration pipeline (remove CommandSet, …

7c721f0

…simplify scanner/registry/tree) Agent-Logs-Url: https://github.com/ikkentim/SampSharp/sessions/f6e0b1a7-d7d3-44a5-9fc5-c6d199902375 Co-authored-by: ikkentim <2820647+ikkentim@users.noreply.github.com>

Copilot finished work on behalf of ikkentim May 9, 2026 18:01

ikkentim added 2 commits May 9, 2026 20:29

make some methods virtual

5b033ff

pass in options

02084e8

ikkentim merged commit 23fda58 into main May 9, 2026
1 check passed

ikkentim deleted the feature/improve_command_processor branch May 9, 2026 18:56

		internal class UnhandledExceptionHandlerImpl: IUnhandledExceptionHandler
		{

Uh oh!

Conversation

ikkentim commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI commented May 8, 2026

Uh oh!

ikkentim May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ikkentim commented May 9, 2026

Uh oh!

Copilot AI commented May 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

ikkentim May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ikkentim May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 9, 2026

Choose a reason for hiding this comment

Uh oh!

ikkentim commented May 9, 2026

Uh oh!

Copilot AI commented May 9, 2026

Uh oh!

ikkentim commented May 9, 2026

Uh oh!

Copilot AI commented May 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ikkentim commented May 4, 2026 •

edited

Loading