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

Consider arguments when literal is impermissible #7

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

hugmanrique
Copy link

@hugmanrique hugmanrique commented Apr 3, 2021

Cherry-picked.
Fixes Mojang#87

hugmanrique added a commit to hugmanrique/Velocity that referenced this pull request Apr 3, 2021
hugmanrique added a commit to hugmanrique/Velocity that referenced this pull request Apr 3, 2021
The vanilla client doesn't know how to serialize the ArgumentTypes used
by the invocation factories (see StringArrayArgumentType).
ArgumentsCommandNode looks like an ArgumentCommandNode<S, String> but
works like an ArgumentCommandNode<S, T>, where T is the type expected by
each Command implementation. This allows for efficient parsing (just one
arguments parse per execution/suggestion).

PaperMC/velocity-brigadier#7 allows us to create
"cheap" hinting nodes, i.e. they have a dummy #requires that returns
false. Before, we needed to set the Command, requirements,
suggestions... This allows for separation of concerns. The clone is
still needed to set this requirement, however.

PaperMC/velocity-brigadier#8 is perhaps the
biggest change: we now use Brigadier redirects for command aliases. This
means we no longer have to perform a deep clone of all hints and the
arguments node, which can save lots of memory for hint-heavy commands
and commands with lots of aliases (e.g. LuckPerms). This comes at a cost
of a reference in each CommandContextBuilder and CommandContext object,
which get allocated for each suggestion to test the context-aware
predicate.
@astei astei merged commit 70c9ac4 into PaperMC:velocity-changes Apr 22, 2021
@astei
Copy link

astei commented Apr 22, 2021

Totally forgot about this. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants