feat: command targeting for nicknamed players - #398
Conversation
Written by GPT so may not be perfect. Allow player-target commands to resolve unique Essential Commands nicknames while preserving real usernames and vanilla selectors.
|
Hi, thanks for working to make EC better. So, I've avoided doing something like this attempts for two reasons so far:
but I will review this implementation to see how/if it addresses the above. (Or maybe if its something that could be toggled in the config.) |
Yeah, no worries! That makes sense. I just wanted a fix for my needs mostly. I know coming from EssentialsX a lot of players enjoy the nicknames. I'll try to get my conversion tool fix for the EssentialsX homes posted and you can check it out. Might be less sensitive since it's a niche use case. |
| // Preserve normal vanilla selector behavior. | ||
| if (target.startsWith("@")) { | ||
| EntitySelector selector = EntityArgument.player().parse( | ||
| new StringReader(target), | ||
| context.getSource() | ||
| ); | ||
| return selector.findSinglePlayer(context.getSource()); | ||
| } | ||
|
|
||
| // Real Minecraft usernames always win over nickname collisions. | ||
| ServerPlayer usernameMatch = context | ||
| .getSource() | ||
| .getServer() | ||
| .getPlayerList() | ||
| .getPlayerByName(target); | ||
| if (usernameMatch != null) { | ||
| return usernameMatch; | ||
| } | ||
|
|
||
| var nicknameMatches = PlayerDataManager | ||
| .getInstance() | ||
| .getPlayerDataMatchingNickname(target) | ||
| .stream() | ||
| .map(PlayerData::getPlayer) | ||
| .filter(Objects::nonNull) | ||
| .toList(); | ||
|
|
||
| if (nicknameMatches.size() == 1) { | ||
| return nicknameMatches.get(0); | ||
| } | ||
| if (nicknameMatches.size() > 1) { | ||
| throw CommandUtil.createSimpleException(Component.literal( | ||
| "Nickname \"" + target + "\" matches more than one online player. Use the real username instead." | ||
| )); | ||
| } | ||
|
|
||
| throw CommandUtil.createSimpleException(Component.literal( | ||
| "No online player found with username or nickname \"" + target + "\"." | ||
| )); |
There was a problem hiding this comment.
Why do we have a separate argument type for nickname-players? doesn't the EntitySelectorNicknameMixin add nickname
support to findPlayers and findEntities, used by EntityArgument?
Does this one have different behavior?
There was a problem hiding this comment.
You're right that there is overlap. The separate resolver was added first for EC commands. The EntitySelector mixin was added afterward to cover vanilla /tp. With the mixin in place, ordinary single-token nickname lookup through EntityArgument.player() should already work.
The remaining differences inside the custom argument are that it supports nicknames containing spaces, adds nickname-aware suggestions and gives you explicit ambiguity/not-found errors. The mixin only affects command resolution in findPlayers/findEntities.
If preserving vanilla EntityArgument semantics is preferable. I'd think the cleaner implementation would be to use EntityArgument.player() everywhere and let the mixin handle lookup, unless support for spaced nicknames/suggestions is something you'd specifically want to retain.
There was a problem hiding this comment.
OK, I think that the nicknames-with-spaces support is nice, but we could also achieve it by stripping whitespace from within the nickname, which I think could work inside EntityArgument. (We are already stripping some forms of disallowed char via getString presumably (like MC's random-text thing)).
Ultimately, I think this feature is best behind a config option, that allows players to choose how "on" this special argument parsing is. I'm thinking something like nicknames_as_command_arg with as an enum config option with possible options of Everywhere, EssentialCommandsOnly, or Never.
That could be enabled by just eary-return-ing in the mixins when it is EssentialCommandsOnly or Never. And by choosing between the vanilla parser and the custom ec$resolveLiteralPlayer in the NicknameTargetResolver for the "essential commands" bit (that way both use the same implementation).
There was a problem hiding this comment.
That makes sense. I agree on the three-state config approach as well as the resolver should be shared rather than having the mixin and NicknameTargetResolver maintain separate lookup behavior.
Let me change it up so Everywhere enables the EntitySelector mixin fallback globally, EssentialCommandsOnly keeps the mixin out of vanilla commands but uses the shared nickname resolver for EC arguments and never stays entirely vanilla.
For whitespace normalization I'll preserve ambiguity handling, since e.g. Foo Bar and FooBar could otherwise collapse to the same command-form nickname. Which might cause problems.
I'll update the PR.
| @Inject(method = "findPlayers", at = @At("HEAD"), cancellable = true) | ||
| private void ec$resolveNicknameForPlayers( | ||
| CommandSourceStack source, | ||
| CallbackInfoReturnable<List<ServerPlayer>> cir | ||
| ) { | ||
| ServerPlayer player = ec$resolveLiteralPlayer(source); | ||
| if (player != null) { | ||
| cir.setReturnValue(Collections.singletonList(player)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The vanilla findPlayers really does a lot more internally than we consider below in $ec$resolveLiteralPlayer. Ideally, we should just be adding onto the existing vanilla results when this is enabled, rather than replacing them, in order to avoid breaking expectations that other in-game systems might have.
This would also allow us to avoid trying to replicate the vanilla behavior like this (we'd just be concerned with our nickname-specific additions):
// Preserve vanilla username precedence.
ServerPlayer usernameMatch = source
.getServer()
.getPlayerList()
.getPlayerByName(this.playerName);
if (usernameMatch != null) {
return usernameMatch;
}Vanilla EntitySelector.findPlayer:
public List<ServerPlayer> findPlayers(final CommandSourceStack sender) throws CommandSyntaxException {
this.checkPermissions(sender);
if (this.playerName != null) {
ServerPlayer result = sender.getServer().getPlayerList().getPlayerByName(this.playerName);
return result == null ? List.of() : List.of(result);
} else if (this.entityUUID != null) {
ServerPlayer result = sender.getServer().getPlayerList().getPlayer(this.entityUUID);
return result == null ? List.of() : List.of(result);
} else {
Vec3 pos = (Vec3)this.position.apply(sender.getPosition());
AABB absoluteAabb = this.getAbsoluteAabb(pos);
Predicate<Entity> predicate = this.getPredicate(pos, absoluteAabb, (FeatureFlagSet)null);
if (this.currentEntity) {
Entity var12 = sender.getEntity();
if (var12 instanceof ServerPlayer) {
ServerPlayer player = (ServerPlayer)var12;
if (predicate.test(player)) {
return List.of(player);
}
}
return List.of();
} else {
int limit = this.getResultLimit();
List<ServerPlayer> result;
if (this.isWorldLimited()) {
result = sender.getLevel().getPlayers(predicate, limit);
} else {
result = new ObjectArrayList();
for(ServerPlayer player : sender.getServer().getPlayerList().getPlayers()) {
if (predicate.test(player)) {
result.add(player);
if (result.size() >= limit) {
return result;
}
}
}
}
return this.<ServerPlayer>sortAndLimit(pos, result);
}
}
}There was a problem hiding this comment.
(we might be able to do this by intercepting the vanilla return value, instead of injecting at HEAD, I think?)
| if (nicknameMatches.size() > 1) { | ||
| throw CommandUtil.createSimpleException(Component.literal( | ||
| "Nickname \"" + target + "\" matches more than one online player. Use the real username instead." | ||
| )); | ||
| } | ||
|
|
||
| throw CommandUtil.createSimpleException(Component.literal( | ||
| "No online player found with username or nickname \"" + target + "\"." | ||
| )); |
There was a problem hiding this comment.
we should use translations when making error messages. See an example here:
and fill in the english translations:
There was a problem hiding this comment.
I pushed a revision based on the feedback. The mixin now runs after vanilla resolution and only adds a nickname result when vanilla found nothing. I also added the three-state nicknames_as_command_arg config and shared the nickname fallback between global and EC-only handling. Whitespace normalization is included with ambiguity protection. The revised code builds successfully with Gradle; I have not live-server tested this latest iteration tho.
Add configurable nickname argument scope and use vanilla-first nickname fallback with shared resolution logic.
Summary
Adds configurable nickname-aware player targeting while preserving vanilla
EntityArgumentbehavior as the primary resolution path.This revision was updated based on review feedback so nickname resolution is an additive fallback rather than a replacement for vanilla player/selector handling.
Configuration
Adds:
nicknames_as_command_argwith the following values:
EverywhereEntityArgument-based targeting globally.EssentialCommandsOnlyNeverThe current default is
Never.Nickname behavior
John Smithcan be addressed asJohnSmith.Validation
The revised implementation builds successfully with Gradle.
The original nickname-targeting implementation was live-tested on Fabric 26.2, but this latest review-driven refactor has not yet been live-server tested and is being submitted for review/testing.