#Brigadier Command API

1 messages · Page 2 of 1

neat radish
#

y

tacit umbra
#

that's the instance of the custom argument type that wraps the nms argument type

#

a custom argument type can be double wrapped

neat radish
#

Yes, but wouldn't putting that check still work?

#

These are custom argument types still (the vanilla arg providers)

tacit umbra
#

ok, yeah. that should work then still

#

Also, I think we should have a way for plugins to "construct" custom argument types from a regular brig argument type and a function converting them to a type. I think that allows further platform-agnostic abstraction

#

you can take a plain brig custom argument type, and create a paper-custom one without dealing with the inheritance stuff

neat radish
tacit umbra
#

I mean basically we just have a simple impl of CustomArugmentType that takes an extra Function<Primitive, Type> function, then the convert method uses that function

#

and then a static method on CustomArgumentType to create it

neat radish
#

@tacit umbra If you wanna whip something up for that, feel free. I've pushed the validator fix.

tacit umbra
#

we should expose the StringReader in the parse method as well actually. it is useful for creating better exception messages, just look at how SimpleCommandExceptionType is used

#

expose it as immutable tho

#

also might be good to add methods to the MessageComponentSerializer to more easily create the exception types? or at least the inner Function interfaces

neat radish
#

I agree

proud timber
#

What about this?

#

Btw I figured out why my suggestions were all wrong, it's because I didn't know that Material was an enum class, and my plugin handles enums differently

tacit umbra
proud timber
#

Well it's used for validating the argument type

#

And it gives insight in to how the argument is used for people looking through the code

#

And also there is no downside of allowing it

ebon hill
#

virtual dispatching is expensive rieRun

tacit umbra
#

The validation happens way earlier than when commands are registered, so it’d have to be delayed or ran again after all plugins have loaded.

actually, command validation isn't run at all on production servers

#

Ok @neat radish talk me through the handleSuggestions method we have in CustomArgumentType. Is there a way we can avoid that? Cause you have to override 2 things if you want to do suggestions instead of one.

proud timber
neat radish
tacit umbra
#

vanilla commands don't need to be validated every time, just before a version release. Cause they don't change randomly

proud timber
#

I see, you are right. I misread DefaultAttributeRegistry.checkMissing() for CommandManager.checkMissing() ```java
public static void logMissing() {
Bootstrap.ensureBootstrapped(() -> "validate");
if (SharedConstants.isDevelopment) {
Bootstrap.getMissingTranslations().forEach(key -> LOGGER.error("Missing translations: {}", key));
CommandManager.checkMissing();
}
DefaultAttributeRegistry.checkMissing();
}

#

That's a bummer tbh, I also could have sworn they did actually check it always. Maybe some versions ago?

tacit umbra
#

we could maybe add support for it, and it'd run validation if you specify a flag so devs could check, but not really concerned with that now, can be added later

proud timber
#

Can the getExamples method still be made available or?

tacit umbra
#

well the downside of removing the final is that it could lead to confusion that it actually does anything

proud timber
#

JavaDocs help with that no?

#

Just some api note

tacit umbra
#

well clearly not, cause you suggested that it be overidable to "document" how to use the argument type 🤣

#

not 100% on it being final. it wasn't originally

#

what is the usecase for it not being final now?

#

I don't want it to be not final, documented as not doing anything, and then suddenly it does something. that would be breaking API contract

#

but if we blocked overriding it, then when it does something, allow it, no contract breaks

proud timber
#

Well is it really breaking API if it actually means someone wrote their argument wrongly lmao

tacit umbra
#

no that's not what I mean

#

If the method is documented as not doing anything or having any effect, that is the API contract for that method

#

maybe mojang adds some use for it on the server then later

#

and suddenly the method that was contracted as "not doing anything" does something

#

if it does nothing, best to not have it, that way if it ever does something, no contract is broken by opening it up to overrides

proud timber
#

Sure

#

But aren't "breaking" API changes necessary sometimes due to Minecraft changing

tacit umbra
#

well generally no, not to our API. but brig isn't our API, its mojangs

proud timber
#

But yea this is a small detail anyway, it's fine like this ig

tacit umbra
#

behavior is broken, and maybe methods no longer function, but changing function generally is avoided when possible

proud timber
#

Btw I also found the doubly overriding thing a bit weird to allow suggestions

tacit umbra
#

yeah, I'm gonna see if that can be avoided. basically, right now its kind of needed to determine if your custom argument should use the suggestions on the primitive argument type, or your overriden listSuggestions.

#

One possible solution is to use reflection to see what type on the type heirarchy the listSuggestions method is defined, but that seems hacky.

#

so if the declaring class for the listSuggestions method on the argument isn't CustomArgumentType, that means it was overriden and should use your custom suggestions

#

if it wasn't overriden, use the primitive args listSuggestions

proud timber
tacit umbra
#

lots do

#

EntityArgument

#

positions, item stack

#

primitive != java primitive

#

primitive == native argument type to minecraft

proud timber
#

Ohhh those

#

I was confused with the default brig argument types

#

Whoops

#

Mmm yea reflection seems the way to go ig

tacit umbra
#

@neat radish so I'm thinking about the parse() and convert() methods in CustomArgumentType. If we allow overriding parse, therefor allowing the custom arg type to parse manually from the string, it's almost worth splitting out to have 2 custom arg types, once extending from the other

#

So basically, if you just overrode parse, the only thing the primitive would be doing, would be providing validation for the argument input on the client, but all parsing would be done by the server, is that something we want to support?

#

then we could have a subtype of that which added the convert method if you didn't want to parse yourself, just convert

#

I can see a use case for both. It would be more performant for a lengthy argument to fail part way through parsing rather than parsing the whole thing, then converting it to your type and seeing if its valid

neat radish
tacit umbra
#

that way, we only expose the reader during parsing, and not during conversion. Having the reader during conversion isn't super useful, cause the point of creating exceptions is to show exactly the point in the argument where parsing fails

#

can't do that if it already has been parsed+converted

tacit umbra
# tacit umbra One possible solution is to use reflection to see what type on the type heirarch... 
// If however we want to handle suggestions ourselves, reference the wrapper.
try {
    final Method listSuggestions = wrapperArgumentType.getClass().getMethod("listSuggestions", CommandContext.class, SuggestionsBuilder.class);
    if (listSuggestions.getDeclaringClass() != CustomArgumentType.class) {
        suggestionProvider = wrapperArgumentType::listSuggestions;
    } else {
        suggestionProvider = null;
    }
} catch (final NoSuchMethodException ex) {
    throw new IllegalStateException("Could not determine if the custom argument type " + wrapperArgumentType + " overrides listSuggestions", ex);
}
#

also @neat radish I'm changing the type of the vanilla argument type wrappers. I don't think they should be "CustomArgumentType" cause they aren't also, that means you can cast them, and get the actual nms argument instance

#

also, the getExamples method should return the nms examples, no reason not to, and can't do that if we have getExamples final in CustomArgumentType

tacit umbra
#

ok, so I noticed some odd behavior, and the cause makes it kind of unclear how to fix, or if we should fix. Take a look at the demo plugin, specificaly try using the /material <item|block> <key> command, but put an "invalid" namespaced key in, so like /material item diAmond (can't have capitals). The error throw is from the "convert" method of the MaterialArgumentType, and the "key" its converting, is mincraft:di. The vanilla ResourceLocationArgument reads until it gets to an invalid char, but the exception for "invalid arugment" is in the parser, if after parsing an argument it finds non whitespace after. The code doesn't get there, cause the exception is throw in the convert method.

#

This could be fixed once registry mod API is in, so we can better use the ResourceArgument which represents something in a registry

#

pushed everything I've done today on it

tacit umbra
#

not 100% sold on the block position argument. You are losing quite a bit of info, like local coords. It is flooring doubles, so you lose precision about the inputted position

neat radish
#

@tacit umbra Lemme look through this all here.

neat radish
#

Maybe the documentation can clarify that you need to handle some cases like that

#

Cause see what mojang does for usages of that argument.

Screenshot_2023-07-24_at_11.27.57_PM.png
#

Ohh wait

tacit umbra
#

yeah, so maybe we recommend not to create a custom arg type for a "Keyed" thing like this directly, but get the key and then parse it to something in the command execution logic

neat radish
#

Are you talking about this too?

Screenshot_2023-07-24_at_11.28.52_PM.png
tacit umbra
#

well yeah, that error doesn't show up for the custom MaterialArgumentType

#

like try the command I showed, the error will just be "Invalid ID" and its thrown in the "convert" logic in MaterialArgumentType

neat radish
#

Yeah hmmm, but the error is there client side

Screenshot_2023-07-24_at_11.29.44_PM.png
tacit umbra
#

correct, cause its just a regular resource location argument

#

rememeber that for keyed registry things, there is actually a "better" argument, the ResourceArgument, but we want the registry mod PR for that, specifically the RegistryKey type

neat radish
#

Oh yeah 100%

tacit umbra
#

so I think we just document that if you want a "keyed" argument, the recommended way is to just have a plain namespacedkey argument, and then in the command execution logic, check if that key is valid

#

that way the error message correctly shows the incorrectly formated key

neat radish
#

also instead of NamespacedKey, we should use whatever adventure type there is instead

tacit umbra
#

I added both. Both should exist until we transition more stuff to Key, cause its annoying to have to create namespaced key instances from adventure instances

neat radish
#

oh I see.

neat radish
tacit umbra
#

so the ResourceLocation argument only parses the input string until it hits an invalid char, in that case, the X, and returns the resource location minecraft:e

neat radish
#

Yeah

tacit umbra
#

then just after it finishes parsing with no thrown exceptions, it checks if the next char is a space ( ). If it isn't, it throws that error underlining the bad input

#

look on Line 371 of CommandDispatcher, just after it parses with no runtime exeptions

#

it peeks to see if the next char is space, if not, throws the dispatcherExpectedArgumentSeparator built in exception

neat radish
#

I see

#

So then are we reading extra chars here then?

tacit umbra
#

well it detects the X and, trips that exception which is then printed in the error message

#

the parsing logic expects the next char after finishing a parse to be a space, if not, it creates that exception

#

and ResourceLocation#read(StringReader) only reads on the reader until it hits an invalid char, then it returns what it already collected

neat radish
#

@tacit umbra Ohh I understand.

Screenshot_2023-07-24_at_11.43.58_PM.png
#

The issue is doing validation logic in the argument, which takews precedent?

tacit umbra
#

correct. it's weird that the resource location doesn't just read until a space, if it finds an invalid char, throw an exception

neat radish
#

Yeah okay, I see.

#

So yeah in general I think that's fine then, lol. Vanilla arguments are gonna be quirky, I was just super confused.

neat radish
tacit umbra
#

can always quote strings tho, to make it take any char

#

or greedy string

neat radish
#

Whether a new type, idk.

#

But that's defo yucky

tacit umbra
#

if you add a Coordinates equivalent, you can do like 5 argument types with it

#

Vec2, Vec3, ColumnPos, Rotation, and BlockPos

neat radish
#

Yeah okay, we should do that then.

tacit umbra
#

a bunch of those are missing dedicated types, but it's not required really

neat radish
#

I think it's good to at least have a generic type for future proofing

#

Are there any other notable argument types we are missing?

tacit umbra
#

we can probably do the ranges using guava's Range type, for int and double range. It'll use the box types instead of primitives, but I think that is fine

#

the scoreboard stuff is interesting, cause afaik the suggestions will be based on the player's current scoreboard right? I don't think the server is queried for that

glass smelt
#

I mean, that makes sense given the entire scoreboard is sent to the client

#

don't think that any of the built in suggestions ask the server? 🤷‍♂️

tacit umbra
#

that's not true at all. it queries the server for a bunch of stuff.

#

sounds, I think it does query for scoreboard objectives actually looking at vanilla src

glass smelt
#

oh

#

here I was thinking they added that purely for us

neat radish
#

I always think this is interesting

Screenshot_2023-07-25_at_12.03.29_AM.png
tacit umbra
#

well those are just a few of them, any "custom" suggestions on a node use the server for suggestions

neat radish
#

Yeah

tacit umbra
#

if a node uses the argument type's default listSuggestions, then it sometimes doesn't ask the server, but look at ObjectiveArgument#listSuggestions

neat radish
#

Oh that's interesting

tacit umbra
#

that second "if" block is triggered on the client, and that queries the server. SharedSuggestProvider#customSuggestions is overriden by ClientSuggestionsProvider to send a packet to the server to query

glass smelt
#

hm

neat radish
#

That seems to be the only argument that does that

tacit umbra
#

lol wat. it was just the first one I looked at. hadn't yet looked at other scoreboard stuff

glass smelt
#

see, afaik most of the stuff relating to the scoreboard is sent to the client

neat radish
#

Oh wait no, that is used in alot of places

glass smelt
#

so, idk what all they do there, would be odd if they queried the server for all of it

tacit umbra
#

they don't for some stuff too. Like EntityArgument, that is usually done on the client. Paper forces it to the server to fix a bug

#

but in vanilla, that doesn't query the server

neat radish
#

So in some cases they handle it client side only, while in some cases they split it? Like see GameProfileArgument#listSuggestions

#

Wait, what, no. That would occur both on server/client right?

tacit umbra
#

yeah it would, but it's not gonna run on the server. A couple uses of the game profile argument have custom suggestions set too, the op and deop commands use server suggestions to get the list of opped or not opped players

neat radish
#

But what makes that not run on the server? CommandSourceStack is Shared too

tacit umbra
#

because the client isn't gonna query the server for suggestions

neat radish
#

Ahhhh

#

Right

tacit umbra
#

when the client is parsing the command input, its gonna get suggestions for that node, not query the server cause ASK_SERVER wasn't set unless there's a custom suggest on the node

neat radish
#

Yep yep okay

tacit umbra
#

the ASK_SERVER is set when its creating the root node to send to the client to replace any custom suggestinos on the node

#

otherwise it'll just use the listSuggestions method built in to the argument type

neat radish
#

But ObjectiveArgument is weird becauase it calls the customSuggestion, right? Which does the server hop

tacit umbra
#

but sometimes even that method queries the server, like ObjectiveArgument

neat radish
#

Yeah so that seems that objective argument is the only argument like that then.

#

Nothing calls customSuggestion like that

tacit umbra
#

well, that's not the only place that customSuggestions method is called on the client

neat radish
#

Wowwy, okay so I didn't know that logic got that in depth in some cases.

#

Ask server, objective argument, and resource key arguments

Screenshot_2023-07-25_at_12.13.38_AM.png
tacit umbra
#

I do not know how that would happen

#

that orElseGet bit

#

if you send an argument for a registry that doesn't exist on the client?

neat radish
#

If it's an invalid registry?

tacit umbra
#

they all should exist

neat radish
#

Yeah that's weird lol

tacit umbra
#

custom registry support? lol. API to create custom registries

neat radish
#

Well that's what I wanted at some point......

tacit umbra
#

actually... I think some of them "don't" exist

#

Ok, so the composite RegistryAccess.Frozen the client uses has all the built ins, and all the remote ones sent in the login packet

neat radish
#

Ahh, do you mean like worldgen registries?

tacit umbra
#

but that isn't all of them

#

yeah, some of the non built in registries still aren't sent to the client

#

I was thinking that all non built in registries were sent the client, but that's not true

neat radish
#

Gosh registries just get more and more crazy

#

Yeah, that's what I thought too

ebon hill
#

a registry registry???

neat radish
#

I guess it makes sense tho

tacit umbra
#

yeah, so only these are sent to the client, all the other non built-ins aren't even on the client

neat radish
ebon hill
#

let's gooo

glass smelt
#

yo bro

ebon hill
#

now make it sync

glass smelt
#

I heard you liked registries

#

so I put a registry in your registry

neat radish
#

Is it just Registries

tacit umbra
#

no, that's all the keys

#

RegistryDataLoader has the full list

neat radish
#

Ahh okay

tacit umbra
#

they are still called "worldgen", but they really aren't cause like chat type and armor trim stuff is there too

neat radish
#

Mmm yes, the worldgen/multi_noise_biome_source_parameter_list registry

tacit umbra
#

yeah, if I thought about where the resource key arg would be used, this would be obvious. can place custom structures with a command

#

structures aren't sent to client

#

so it had to query the server

neat radish
#

Mhm

tacit umbra
#

back to scoreboard arg types, so it looks like we can add Objective. It would be interesting if we modified the vanilla arg slightly to accept any scoreboard, not just the main one

#

so you could create an objective argument for a custom scoreboard...

neat radish
#

Scoreboard api 😟

tacit umbra
#

could do operation argument, its not too crazy

#

could do angle, just making it an ArgumentResolver<Float> essentially

neat radish
#

I think those range arguments would be nice, the client side completion is nice for those.

#

Maybe TimeArgument too

tacit umbra
#

no clue what the swizzle argument is

#

is swizzle a technical term? its used in the execute command, which like 95% of its functionality is foreign to me

tacit umbra
# neat radish Scoreboard api 😟

nah, don't think its a good idea anymore. The TeamArgument seems to only rely on the client's known team names rather than ask the server

#

oh that is annoying tho... cause it breaks the commands for any player that might have a custom scoreboard set

#

entity anchor.. don't you have that for the teleport api stuff?

neat radish
neat radish
tacit umbra
#

well we could make helpers to get it.. ugh generics suck with brig, why didn't you just use type tokens MOJANG

#

also, there aren't any completions for it. the completions you are thinking of are in the selectors

#

but it works

neat radish
tacit umbra
#

yeah, that works

glass quest
#

will /execute be able to run plugin commands with these changes?

ancient wolf
#

Or at least it used to o.O, maybe something changed as it doesn't seem to say that in the PR description

neat radish
#

It does

mellow jolt
#

And command blocks, from my testing :)

proud timber
#

I'm back from my holiday, I'll do some testing later

mellow jolt
#

whenever this needs to be documented, mentioning that all commands now work with /execute and command blocks will be quite important i think
using execute as @e run command might run the same command many times very quickly etc
including existing non-commandsourcestack commands that just use get sender (where it would run over and over)

proud timber
#

Is there any ETA for the merging of this PR?

glass smelt
#

no

proud timber
#

Longer or shorter than a month?

#

If you had to guess

glass smelt
#

I can't guess

#

We generally don't offer ETAs on stuff

#

idk if it's mergable right this second or not

#

and personaly, I'm kinda snookered

proud timber
#

How long does a PR of this size generally take you'd say?

pure vesselBOT
#
__There Is No ETA__

Updates to Paper do not have any sort of estimate for when they release, ever. Any and all updates will arrive when they are ready, and the only thing to do is wait for them patiently along with everyone else.

ETA means 'Estimated Time of Arrival'!
proud timber
#

Oh and I just wanted to say that this api feedback channel is an amazing idea, talking in #paper-dev about Paper's own api (as the channel description actually suggests you do) would have been very annoying

proud timber
#

I think the issue is still present

proud timber
#

May I also suggest allowing LiteralArgumentBuilders as inputs to register? Vanilla also has it like this

#

It will let you omit that build call at the end of the command, which is kinda nice

tacit umbra
#

when I have the permission for the sub_command, it shows in suggestions

#

when I set it to false via luckperms, it goes away

proud timber
#

Could you try my exact command

#

This one

#

It should definitely not show sub_command

tacit umbra
#

you mean check the sender name?

proud timber
#

Yeah I mean anything that's obviously false

#

From the beginning

tacit umbra
#

doesn't show any suggestion for me

image.png
#

if I invert the requires check, it shows as my username isn't xpple

proud timber
#

This is my exact code java this.getServer().getPluginManager().registerEvents(new Listener() { @EventHandler public void onLoad(ServerResourcesLoadEvent event) { event.getCommands().register(TestPlugin.this, Commands.literal("root_command") .then(Commands.literal("sub_command") .requires(source -> source.getSender().getName().equals("xpple")) .executes(ctx -> { ctx.getSource().getSender().sendMessage("You are xpple!"); return Command.SINGLE_SUCCESS; })).build()); } }, this);

tacit umbra
#

and what do you want me to do to replicate the issue?

proud timber
#

And it shows

image.png
tacit umbra
#

is your username xpple?

proud timber
#

But then says

image.png
proud timber
tacit umbra
#

ok, well mine isn't either, and I do not get a suggestion

#

make sure you are up to date

proud timber
proud timber
tacit umbra
#

make sure the server is up to date I guess, not just what shows in your IDE

proud timber
#

Oh fuck me

#

Lmao sorry

#

I forgot I had to manually update the jar for my testing server

#

Yeah that will undoubtedly solve the issue

#

Yep, solved

#

Sorry for the inconvenience and thanks for the fix :)

tacit umbra
#

ok and yeah, right now plugin commands don't seem to override vanilla ones, that will be fixed

tacit umbra
#

ok, that's fixed.

#

we have to determine what the best way to deal with argument types that have generics

#

brig's CommandContext doesn't support any type token, just Class which is only used for the generic to cast, and to check if the parsed argument value is an instance of that class

#

no vanilla argument has a generic, but we use a generic for several arguments that require the context to resolve the final value

#

Should we just make concrete classes for each of those? it'd be like 6 of them

#

SingleEnttity
MultipleEntities
SinglePlayer
MultiplePlayers
GameProfiles
BlockPosition

#

or we have some helper method to get the parsed value from the context like currently exists on CommandSourceStack (getResolvedArgument)

proud timber
tacit umbra
#

yeah, but they don't do any unchecked casts, well not directly. you have to do an unchecked cast somewhere

proud timber
tacit umbra
#

in what argument class

proud timber
#

VanillaArgumentTypes

tacit umbra
proud timber
#

It's normal in vanilla to have get<Argument> methods anyway

tacit umbra
#

can get away with 1 cause those arguments can share a parent type, ResolvedArgument

#

yeah, but those are on the argument type, not clumped all together as static methods on one util class

proud timber
#

Yeah that's an unfortunate side-effect of having them all in VanillaArgumentTypes but I don't think it's a problem

#

But, really, I don't see the point of adding lots of classes

#

Tbf it kinda makes sense to have the get<Argument> methods since they complement the <argument> methods well

tacit umbra
#

at least what I could think of

proud timber
#

Nice!

#

Also nice that you took notice of my suggestion from above :)

mellow jolt
#

maybe an offical way of changing the fallback prefix in the new api? cause having a fake plugin instance that just overrides name (and isnt registered at all) works, but seems a bit too hacky?

#

idk

proud timber
#

What fallback prefix?

mellow jolt
#

like if the plugin is named myplugin, then mycommand is registered as /mycommand and /myplugin:mycommand

#

there are methods on the commandmap to register with a custom fallback prefix, but not in this brig api

#

it works if you have custom plugin interface that returns something else for getMeta().getName() (or whatever it is), but then there is no longer a correct refference to the registering plugin, which may break others (say a /help thing that gets more info about the plugin from it)

tacit umbra
#

I don't think we want to support that?

#

the prefix should be the plugin name

#

that's the whole point of the prefix

mellow jolt
tacit umbra
#

you pass in that other plugin's instance then

mellow jolt
glass smelt
#

that's been exposed for years

#

but

#

bearing in mind that upstream never exposed the means to interact with the command map for the past decade

#

it's basically quasi internal stuff

mellow jolt
tacit umbra
#

yes, that is not supported API

#

PluginMeta is marked as non extendable

#

still don't know why you want a custom fallback prefix

proud timber
#

I kinda understand, the plugin's name may not always be suitable/preferable for the prefix. I personally dislike the fact it's capitalised for example. Namespaces are generally lowercase

#

But ig the fallback prefix is not actually a namespace technically

mellow jolt
#

capitalised?

#

what is?

proud timber
#

/TestPlugin:command

#

As opposed to say /testplugin:command

mellow jolt
#

oh if the plugin has capitals

#

rather than test-plugin

proud timber
#

Yes

#

I think people tend to capitalise the plugin's name

mellow jolt
#

wait can you have 2 plugins differing in only case loaded?

#

or are ids compared case insensitive and its just for display

tacit umbra
#

but I do not think it should be dynamically set

mellow jolt
#

funni unsupported "api" go brrrr

proud timber
glass smelt
#

it's literally just there to provide a stable name for people to deal with

proud timber
#

More of a Bukkit thing I suppose

mellow jolt
proud timber
#

How do the custom argument types work behind the scenes? Are they just remapped to their parent type when sent to the client?

#

Cause like, how would that work with custom overridden parse methods (the converted ones are obvious)

mellow jolt
#

basically they always parse as valid client side iirc (assuming your validation is stricter)
like i tried to do a literal thing, and i only got the red text while completing when it wasnt a valid string (mismatched quotes iirc), even if it doesnt parse, and running the command gives an error

tacit umbra
proud timber
#

Yeah that's what I expected but that feels very wrong

glass smelt
#

at the end of the day it is kinda wrong

proud timber
#

Non red text giving command errors

glass smelt
#

but, it's a common feature used by command frameworks

proud timber
#

Doesn't sound intuitive

glass smelt
#

Well, it's not, but such is the limitations

tacit umbra
#

I mean there's no way for the text to be not red without a client mod

glass smelt
#

ideally mojang would let the server give some feedback

tacit umbra
#

also, that's how its been forever pretty much

ebon hill
#

if you want the real types with the real parsers to exist on the client as well, they need to exist on the client as well, as well as the network registrations etc

proud timber
#

I know

ebon hill
#

you can only do so much from the server

mellow jolt
#

brig limitations :(

proud timber
#

But like

proud timber
tacit umbra
#

its not a brig limitation, its a "only modifying the server" limitation

#

not new, and not exclusive to "bukkit"

proud timber
#

The converted ones at least give you a little bit more hope non red text will be parsed correctly, but with custom parse methods that safety is completely gone

#

What is the reason for custom parse methods anyway

tacit umbra
#

what is the difference where you throw the error?

glass smelt
#

Because it's a common part of command frameworks essentially

tacit umbra
#

can just throw it in the execution, there was no red text there

proud timber
#

I'm trying to say, what is the point of sending the client some argument type that you promise the client is what you'll be using, only to use possibly completely different parsing logic? Basically in what situation is a custom parse method better than a convert method

#

I'm aware that the convert method may fail the same way

glass smelt
#

the entire thing is that the brig systems strengh is that it maps types during parsing

#

take Location

#

How do you define a location, how many args does it take?

#

Now, you could have "world x y z", or, "x y z", both of those generally being parsable based on the current context

mellow jolt
#

if i wanted to make a fancy location parser, then
3 or 4 or maybe 1 (here)
(but 1 "arg")

glass smelt
#

ofc, the way the client will treat that will kinda suck, but, that needs to be boiled down into a parsable type

#

otherwise you just go back to the old classic of treating everything as a giant ass long string, and then you lose the advantage of brig in general

#

ofc, we are operating within the bounds of limitations, the client doesn't understand this stuff, so for a location you'd just try to conform to the types that vanilla understands where viable

#

but, variable length stuff is a key area where being able to override the parse method to have some control over the args is key

mellow jolt
#

in fact thats what vanilla did for clone, right? they introduced an optional "from <world>" thing instead of a location with a world as 1, right?

#

like using the literal as a marker

proud timber
tacit umbra
#

Location is a great example. I want them to enter a location that is above them. Block pos argument is great, but it won’t show “red text” for positions below. If I use the custom parse, and can set the string reader pos to exactly the error which then shows a more helpful error message than some generic one

#

If I just used the converted one, I couldn’t show as specific an error

glass smelt
#

Well, for a native type you'd sadly need to make it a greedy string, given the lack of shared understanding

tacit umbra
#

I could still throw an error, but it wouldn’t have the string reader in the correct pos

glass smelt
#

it's just an ez example though if you don't wanna bake it into multiple seperate args

proud timber
glass smelt
#

as said, greedy

proud timber
#

Yeah right

#

So you don't need a custom parse method?

glass smelt
#

it's a nice util feature

tacit umbra
#

It would be useful, cause you have a reader, and can use the brig exceptions

glass smelt
#

it's not perfect, but it's a common thing that commands want to be able to do

tacit umbra
#

You can’t do that in “convert” cause it’s already parsed

proud timber
#

Wait I think I know of a different use case

#

Suppose you want a block argument type. You use block predicate as parent argument type and override the parse method cause you don't want a predicate. That way you have a perfect block argument type that is 100% consistent with the client

#

:einstein:

#

Perhaps add this as an example I think it illustrates the power of a custom parse method very well

proud timber
#

Where are the custom argument types remapped internally?

proud timber
#

What do we do with arguments that require command build context?

tacit umbra
#

can I get an example

proud timber
#

Block predicate

#

Ideally the command build context is supplied in the event or something?

#

So I guess we'd need a separate event for commands

#

Idk if that's applicable here, sorry if it's not

tacit umbra
#

@proud timber I don't think there is a point right? At least not yet. The vanilla args are constructed behind the scenes so you don't need it to create any of the arguments in VanillaArgumentTypes

proud timber
#

Yeah not yet, but what if we want to expose the block predicate argument as well for example?

#

Or registry related ones

tacit umbra
#

it would just use it behind the scenes to create it

#

why would the event expose it only for you to use it? why not just make the method VanillaArgumentTypes#blockPredicate use it in its impl

proud timber
#

Is that possible? Where do you get the instance of it from

tacit umbra
#

yeah, its held in a field in ReloadableServerResources

#

either way, if its needed, we can add it later. THe event is changing already, its just not in there yet. the command regsitration event is probably gonna move over to a lifecycle event, the system for which can be found here

GitHub
Lifecycle Event System by Machine-Maker · Pull Request #9629 · Pape...

A separate and minimal event system is required for lifecycle-related registration, such as commands, the future Registry Modification API and other similar resources which are either registered on...

proud timber
#

If so, what is the point of passing it around everywhere as happens in vanilla

tacit umbra
#

I mean that's how you use it? it uses that field when it creates nms.Commands

#

it doesn't have a getter, and its a private field. the ReloadableServerResources is re-created during a /minecraft:reload and a new one is created

proud timber
#

I'm saying how do you get that field without having an instance of ReloadableServerResources

#

I should probably have a look at the code

tacit umbra
#

you can get the instance from MinecraftServer, there's a field for it there

proud timber
#

And how do you get an instance of minecraft server lol

tacit umbra
#

MinecraftServer#getServer a static method

proud timber
#

Wut does that exist

#

Hold on

tacit umbra
#

not in vanilla

proud timber
#

Ah that explains it

#

Fair enough

#

Man that should exist in vanilla btw

#

Would make life much much easier

#

But anyway yeah that solves it

tacit umbra
#

also, the suggestions thing you asked for on the PR... my one reservation is that they are opinionated. We aren't trying to create a command framework with any opinions, just expose brig as directly as possible

proud timber
#

Yeah that's true, I should keep that in mind better

#

It's just that when I think Brigadier

#

My mind goes to custom argument types and everything

#

For which these helper methods would be very useful

tacit umbra
#

and you can create them yourself, exactly as you want them

#

like the string matching in SharedSuggestionsProvider special cases _, checking if your string matches either the beginning or any text after each _.

#

that is an "opinion", and I don't really think it belongs

#

side note: @neat radish should we make CommandSourceStack implement ForwardingAudience?

#

vanilla's CommandSourceStack has "sending messages" methods

neat radish
proud timber
proud timber
#

Does that interface also have something to broadcast to ops?

#

That could replace source.getSender().getServer().broadcast(component, Server.BROADCAST_CHANNEL_ADMINISTRATIVE);?

tacit umbra
#

That’s be a separate method, but one to consider adding. Audience is just all the message, action bar, title, etc stuff that CommandSender implements now

proud timber
#

Right okay

proud timber
tacit umbra
#

Might have to change that, can’t look now but it’s probably missing something.

proud timber
#

Yeah

proud timber
#

Shall we discuss the todo list some time?

mellow jolt
#

would something like

    @NotNull
    public org.bukkit.command.CommandSender getTargetBukkitSender() {
        if (this.entity == null) { return getBukkitSender(); }
        return this.entity.getBukkitSender(this);
    }

in CommandSourceStack.java be useful?

mellow jolt
#

also is overriding commandsourcestack api?

#

ie to add custom context stuff

stuck trail
#

Without understanding too much about this, that sounds like a bad idea. If adding context is something people need, we should have API for adding context

mellow jolt
#

i had the idea to implement custom settings for commands using a custom css, with a command like execute to modify context from the default
(like
plugincommand with-setting <key>=<value> and output.verbosity=verbose and output.colours=minimal run debug or smth)

stuck trail
#

Overriding shit in the API always becomes a hassle because new stuff can't be added as easily

mellow jolt
#

yeah i get that

proud timber
#

Could this get some attention? Let's just go through the todo list

#

And could we discuss publicly hosting builds of PRs with the label build-pr-jar on some maven server?

glass smelt
#

build-pr-jar doesn't publish to maven for various dozens of reasons, managing that would just be ass

proud timber
#

Why?

glass smelt
#

Because we have no mechanism of uniqifiying the IDs?

#

given how rarely stuff like us actually needing to publish an artifact for, we have no mechanism for it

mellow jolt
#

Could it be possible to add pr number to the artifact I'd published with paperweight to the ci?

glass smelt
#

We have no sane way of doing that atm

mellow jolt
#

Yeah I have no idea what I takes for it to be secure

glass smelt
#

publishing transient builds to the maven server is a pita because removing them generally becomes a headache

#

not to mention the headaches over this being such an infrequent requirement makes it somewhat of a non-ideal arrangement

#

a solution would be nice, ofc, but I don;t think shoving unstable builds into our maven repo is really a good call

proud timber
#

It's definitely doable

glass smelt
#

It's more the "we don't want to persist them for forever"

#

It's doable

#

it's just an entire bunch of headaches over how we wanna figure to set this stuff up

proud timber
#

Yeah I understand

#

It's just that using PR branches is now quite tedious

glass smelt
#

it's on the list of stuff we want

#

it's just we need to figure out a way to do it which doesn't compromise the main repo

#

and then setup a way to be able to inject pr info into the thing which is a headache on gradle

stuck trail
#

It's really not worth the effort generally

glass smelt
#

and then we also need to setup a policy for it so people don't expect that we care to keep those builds around, etc, etc, etc

stuck trail
#

The build PR Label was created so that issue reporters could validate the fix

#

It serves a need for us a maintainers

glass smelt
#

maybe when we start publishing a dozen bunch of PRs like this which require more plugin dev effort, it will make sense for us to drop everything to start drafting such a set of policies and creating a repo for it, etc

#

I mean, even that becomes an entire set of issues

#

there is no real way to do this securely without basically rolling our own magical maven repo stack eqsue thing

proud timber
#

Maybe we can copy the approach of some other repos that have done this

glass smelt
#

idk any other repo which has been stupid enough to setup such a mechansim

#

most people would just setup CI for the branch and remember to publish it under a different version, etc

stuck trail
#

Just jitpack it

#

runs

#

I mean, having a service that emulates a maven API and builds PRs on demand could be a fun project

#

Wouldn't actually publish anywhere, just cache

glass smelt
#

that was one of many thoughts I had

#

could just run publishToMavenLocal and then setup that as a maven repo tree

#

I feel like we just accidently gave mini a distraction from hangar

proud timber
glass smelt
#

that only works for branches owned by us

#

and requires a bunch of leg work to setup a maven repo

#

setup the CI system to build it

proud timber
proud timber
glass smelt
#

Because we do not have the ability to setup teamcity for every single repo people desire a build for

#

It's not difficult, it's just a bunch of work

#
  1. TC only gives us so many build configurations that we can have, not to mention the security implications of running 3rd party builds within our ecosystem
  2. Gradle does not offer a convenient way in which to override information in the build output in a reasonably secure manner that gradle does, i.e. the version information, etc
  3. We would need to figure out a reliable manner of hosting temp repositories in order to manage this mess, including figuring out a reasonable policy in which we're not expect to care about hosting such builds for the next 200 years
proud timber
#

This is what I have in mind, which is probably a naive oversimplification:

  • Have a separate maven repo (something like https://repo.papermc.io/repository/maven-snapshots/)
  • If PR has build-pr-jar, then execute snapshot publish script defined in build.gradle
  • The script would redefine the build version to be the commit hash
  • The script builds the PR and publishes
stuck trail
#

We don't wanna spam our repo with builds like that

proud timber
#

Only the Paper team decides what PR gets build-pr-jar though

glass smelt
#

gradle offers no sane mechanism to define a build version override

proud timber
glass smelt
#

We also cannot/will not run that stuff within our own production build setup

proud timber
glass smelt
#

modifying the version info in a script just sounds like a fragile insecure mess

proud timber
#

?

#

You'll have to resort to commit hashes

glass smelt
#

There is no flag to do it, we'd need to modify the actual build script

#

in which we introduce an entire set of headaches over security

proud timber
#

I'm by no means a Gradle expert but you can just do version = ...

glass smelt
#

how?

#

How do you define that version in a build in which you do not control the config for?

#

We would need to expose environment variables or something for it

#

but, then you introduce a bunch of risks over the fact that people can set that stuff in the build config

#

it's an entire set of headaches which we'd need a proper isolationary solution for, as was already proposed

proud timber
# 
publishing {
    publications {
        mavenJava(MavenPublication) {
            version = ...
            // ...
glass smelt
#

missing my point

proud timber
#

Ah I see what you mean

#

Nexus somehow does it for example

glass smelt
#

maven offers a flag for you to do that

proud timber
#

Ah

#

Wait so what's the problem?

glass smelt
#

We have literally 0 mechanism of injecting overriding version information in a manner which doesn't pose a huge obvious gaping security risk

river flame
#

otherwise only with manual approval

#

so you put all buildscripts in the no-go filter

glass smelt
#

No idea if TC supports that, but its' 100% not something I wanna contend with

#

We have an idea of how we can maybe wrangle something together

#

it's not the full picture, however

mellow jolt
#

Specifically, I publish the dev bundle under the group
io.github.the456gamer.paper-build-pr.<pr number>.paper
Where pr number is a pr with the build jar label

glass smelt
#

No, it's obviously not secure

mellow jolt
#

Actually nvm I've just realised I'm publishing paper-server somehow, let me take it all down

#

Oops

glass smelt
#

relying on sed in order to "safely" replace information in a build file is deeply flawed and is exploitable by a child

#

The only way I could ever see this working is to basically publish to maven local and then host that

mellow jolt
#

Ok the repo should be private now

mellow jolt
#

Either way, for me it was a cool poc

glass smelt
#

No, it changes nothing

#

social engineering is a fun tool

mellow jolt
#

Btw how does paper publish to maven so that paper-server isn't included? I'm guessing it's not publishAll... as that's what I used

#

Is it done by publishing the Dev bundle separately to API /mojang API?

glass smelt
#

we just use the publish task

#

some property thingy too to get it to publish the dev bundle

mellow jolt
#

yeah i saw the property to publish with dev bundle.

is changing the group for within the dev bundle not needed? that would remove one of the "sed"s, and i wasnt 100% sure abount how paperweight treats it

instead of seding to modify within, is appending at the end any better?

#

huh publish seems to publish paper-server as well. does it get filtered repo-side for you, or do you publish sub project separately

glass smelt
#

I wanna say that we just publish the subprojects or something, I don't remember 100% and too dizzy to care to peek

mellow jolt
#

ok ive made the repo public again now that its not going to get me in trouble with microsoft

mellow jolt
#

https://github.com/the456gamer?tab=packages&repo_name=paper-build-pr
it published (havent actually tested using it yet, but oh well)

it still feels like im giving any pr's i build rce, since i am. its just how gradle works as a build tool. as far as i know, the most it can do is delete the other packages in the same repo, ruining it for everyone (or maybe replacing all packages with a malicious version :/)

glass smelt
#

Which is why the only safe approach ever is pretty much going to be ephimural repositories

mellow jolt
#

but it wont auto-build any new versions

proud timber
#

Woah that's cool

mellow jolt
#

im suprised the dynamic matrix builds worked tbh

#

and that it lets me run all 16 at once

#

apparently its limited up to 256 per matrix

proud timber
#

Does the PR need updating for 1.20.2?

neat radish
#

Tbh, this will be sitting around for a bit longer until it gets more attention. Just how things work here.

#

People are busy 🙂

proud timber
#

Understandable

#

Is there anything I can do in the meantime?

hoary prairie
#

tbh

#

I really like the velocity bridgiar api

#

idk if thats default or what but its really nice

river flame
#

it has some slight improvements over standard brigadier but is still quite similar

glass smelt
#

velocity takes over the entire dispatcher, and so it's able to do things like replacing stuff and providing more raw access, vs paper which needs tl deal with the impl vs api seperation

river flame
#

Any chance of a merged PR under the christmas tree? 🎄

#

Can we help in any way?

neat radish
#

This PR is blocked by the lifecycle api PR, if you would like to look at it. 🙂

proud timber
#

Sad

neat radish
#

We’re looking at the life cycle api this weekend

#

So, you’re in luck!

proud timber
#

Niceee

neat radish
#

Brig API updated a bit

proud timber
#

Cool

tacit umbra
#

@neat radish found another structural issue with the server-side of the command registration stuff. It is designed to only exist when MinecraftServer exists. The ApiMirrorRootNode gets the dispatcher from MinecraftServer which doesnt exist when the bootstrap side of the event is gonna be fired.

#

That event is probably gonna be fired in ReloadableServerResources#loadResources

neat radish
#

Hmm okay, thanks for info. Can fix

proud timber
#

What's the state of this PR currently?

#

And where can I find the implementation of the VanillaArgumentProvider?

granite hound
proud timber
#

Yeah but there have been some recent commits

proud timber
covert wren
proud timber
#

Wth why couldn't I find that

#

Ah I probably didn't expand the git diff

#

A new CommandBuildContext is still being created

#

It should probably use the existing command build context

proud timber
neat radish
proud timber
#

Yes

neat radish
#

don't see the issue

proud timber
#

I don't think you're supposed to create a new instance of it

#

Might cause sync issues?

#

I think you can get it through ReloadableServerResources

tacit umbra
#

why? it doesn't hold any state

#

the problem is using the VanillaRegistries, not the creating a new instance of it

#

I actually think I've mentioned this issue before?

proud timber
#

Kinda

proud timber
#

But okay then that should be fixed

tame hawk
#

my god, please add this

#

don't commands registered using the commandexecutor convert to brigadier too?

glass smelt
#

Those commands are registered into the bukkit command map which is then basically copied over to the brigadier one

proud timber
#

May I request a status report?

stuck trail
#

No, you aren't the manager, lol

river flame
#

hi, i'd like to speak to the manager please

proud timber
glass smelt
#

one of the dependencies is going to be merged today

#

but as per usual, no ETAs, etc

proud timber
#

Just wanted to know the status

proud timber
#

That is the lifecycle event system I suppose?

glass smelt
#

yes

ancient wolf
#

Is there anything we (I.e. random users lol) can do to help with this PR/getting it merged sooner? assuming it's just testing, is there a specific part that testing would be useful for (e.g. testing compatibility with current command systems vs testing the new API)? or just generally try stuff out and see if they work as expected?

proud timber
#

I tested the new API and found a few bugs, so I think the API could use some more testing

peak charm
proud timber
#

Because I reported them here

peak charm
#

That was two months ago

proud timber
#

Yes?

peak charm
#

I see some of them were dismissed as not actually a problem, have you checked whether any of them are still a problem?

#

This is why using Discord for bug reports sucks, no one has any idea what the current status is 😛

proud timber
#

They were all fixed

#

Iirc

#

If they were, it was probably mentioned here that they were

#

These were fixed I think

peak charm
#

oh jeez that was even further back

#

So yeah, those should have been comments on github

neat radish
#

@ancient wolf @proud timber The best thing you can do is test this with existing setups. Make sure all bukkit commands work the same

proud timber
#

That has already been tested by many though

#

Maybe not recently ig

peak charm
#

The actual brig API is going to be marked as experimental for a while, right? So if some of the details for it are wrong that's not too big of a deal. That means the main thing would be to ensure existing non-experimental API still works

neat radish
#

Yeah, legit just plop the jar in your server and if there are any changes they should be reported.

proud timber
#

Okay

#

I won't be able to help then sadly

proud timber
neat radish
#

Last updates have been done

solemn ferry
#

Wanted to let you know that when switching to use brigadier, it works perfectly until you reload. When using the .suggest() method in the command, it doesn't work after a reload and requires a restart to fix.

My code:
https://github.com/Dueris/GenesisMC/tree/origin/src/main/java/me/dueris/genesismc/command

GitHub
GenesisMC/src/main/java/me/dueris/genesismc/command at origin · Due...

Custom Origins Plugin for PaperMC. Contribute to Dueris/GenesisMC development by creating an account on GitHub.

neat radish
solemn ferry
neat radish
#

Oh, interesting...

solemn ferry
#

Only after reloads tho

#

The rest of the time it works perfectly fine

neat radish
static owl
neat radish
#

By reload, do you mean minecraft:reload or bukkit:reload

#

@solemn ferry

solemn ferry
#

Sry for the late reply, was getting pizza in the oven lol

neat radish
#

No stress

solemn ferry
solemn ferry
neat radish
#

Could you potentially double check that ur properly updating those fields?

There's a chance that it is maybe referring to an old instance of your plugin or something...
Maybe try replacing those with a static list to test if this is a conversion issue.

solemn ferry
#

Look on line 83, it's a ResourceLocation argument that pulls from the registry inside the plugin that suggests each "tag" to the suggestion builder

#

Upon reloading it doesn't suggest those tags anymore

neat radish
#

Ohhhh

#

Oh wait, yes that's what I am referring to

solemn ferry
#

Everything in the cmd still works tho after reloading, except for the suggestion.

neat radish
#

Try replacing that with a dummy list. Because what I am saying is there is a potential that reloading causes CraftApoli.getOrigins(). to refer to an old instance

solemn ferry
neat radish
#

Like for testing, make it builder.suggest("example") or somethin

solemn ferry
#

Now that I think abt it, that does make a lot of sense.

neat radish
#

That's kinda the ugly nature of reload

static owl
solemn ferry
# neat radish That's kinda the ugly nature of reload

During reloading I could "update" the map by splitting the command suggestions and the origin registry and during disable, clear them both, and on enable I could update the one for the cmds(making the brigadier cmds point to the cmd suggestions map)

solemn ferry
solemn ferry
#

im on my pc now

#

working on testing rn @neat radish

#

@neat radish it does seem to break on reloads

#

before reloading

image.png
#

after reloading

image.png
#

"sdf" was a suggestion i gave teh builder through "builder.suggest()"

#

after reloading, it didnt display

neat radish
#

Hmm okay let me test

solemn ferry
neat radish
#

nah I should be good, will test with a local command

solemn ferry
#

oki doki 👍

#

lmk if u need me to test anything

charred stirrup
#

what's your favorite species of icecream?

image.png
neat radish
#

lol

wise orbit
#

will try this tomorrow, try and make my plugin commands look cool there lol

neat radish
#

@solemn ferry and so ur loading ur plugins from here?

Screenshot_2024-02-17_at_5.31.44_PM.png
#

*commands

solemn ferry
#

Yes

#

The check for if they were null was to see if it was bc the cmd was overriding the previous one during reloads. It always returns true tho

#

Bc it's not in the cmd map

#

It was a theory tho lol

neat radish
#

@solemn ferry So the thing is what you are doing isn't exactly supported

solemn ferry
#

how so?

neat radish
#

Ur using NMS to directly insert commands into the dispatcher

solemn ferry
#

yeah? I thought that was how it worked lol

#

im assuming I did something terribly wrong

neat radish
#

yes but, it's a little different using my API

solemn ferry
#

Ah, i see

#

i thought brigadier had already been inside paper, since the brigadier-related classes were in there(unless im totally wrong rn which i probably am)

neat radish
#

But the big thing here it seems is that current api doesn't support reloading.

#

@tacit umbra Do we want to fire these lifecycle events on bukkit reload

neat radish
solemn ferry
#

So how would I change it to use your api lol

tacit umbra
#

it should already I thought

#

it fire during minecraft reload

#

and bukkit reload fires that

neat radish
#

which is why I am confused

#

Does reloading clear lifecycle events?

#

Maybe this has to do something with the ordering of things?

tacit umbra
#

it does clear them, but onEnable is called again, so it should re-register them

neat radish
#

But is onEnable called before the life cycle events get called

#

(in reload)

#

Because at least the commands aren't showing up and there isn't anything running it seems

tacit umbra
#

oh its possible we need to move the firing of the JavaPlugin lifecycle event, yeah. Its fired at the end of the minecraft:reload sequence. But if that happens while plugins are unloaded...

neat radish
#

If you potentially have time to look into that I would greatly appreciate

solemn ferry
#

@neat radish how would I go abt changing my current system to work with your api?

neat radish
#

it's a bit hard

#

Since we rn can't

solemn ferry
#

That's alright

#

Ty for ur help

#

I'm adding an issue report inside my repo to remind me to update to ur api when it releases

tacit umbra
#

Ok, I pushed a fix for the bukkit:reload command issue

#

keep in mind, that if you register commands inside the plugin bootstrap, any bukkit reload will throw an exception and not reload anything

solemn ferry
#

👍

#

May I ask why it would throw?

tacit umbra
#

bukkit:reload requires unloading all plugins. The pluginbootstrap is not called again when the plugin is loaded back up. So any event handler you register there will just be erased and not fired again.

#

the benefit of registering commands in PluginBootstrap vs JavaPlugin is that commands registered via bootstrap are accessible in datapack functions

#

but that's pretty much the only difference, so if for some reason you still want to use bukkit:reload, registering your event handler in JavaPlugin#onEnable will be fine

solemn ferry
#

Alrighty. Ty for ur help!

#

One last question, would moving the command registration(the nms way that I currently have) to the plugin bootstrap fix this issue? If not that's fine, I understand it isn't rly supported XD

tacit umbra
#

idk what your issue is

#

the issue with bukkit:reload was just fixed

solemn ferry
#

Oh

#

Mb XD

#

I got confused on what you meant for a sec

#

My apologies

#

Ty for your help

golden crypt
glass smelt
#

We don’t have full control over write contexts to rhe thing, and so afaik using a lock is not really an option

golden crypt
#

Ah, writes don't always happen via API calls?

neat radish
#

Indeed

golden crypt
#

I suppose the lock could be accessible (or at least interactable) publically. API stuff would be controlled to avoid CMEs, and if anyone wants to do their own read/write outside of the API it'd be their responsibility to use the lock properly.

river flame
#

Is ServerResourcesLoadEvent still the correct event to listen to? It seems to not be triggered

neat radish
#

No

#

See the example, it uses the lifecycle event system now.

river flame
#

in the PR?

#

which example do you mean

river flame
#

oh i think I found it

#

why do you force push the branch instead of just adding more commits?

neat radish
river flame
#

im just asking because i'm trying to maintain a kind-of downstream of this PR

#

with my own publishing stuff ontop so that I can develop with it

#

and its a bit more annoying to have to force push all the time

#

and cherry-pick my patches

neat radish
#

@river flame Well again, you aren't really meant to do something like that... it is pretty standard to force push like this, so I am sorry it's not working well with your setup here.

What I really would instead encourage is using a standard paper fork, but then just downloading the patch that is on the PR and importing it yourself. (As most of the changes are isolated to two patches)

#

And then whenever you feel like it you can redownload my patch to update.

river flame
#

also so that the CI pipeline with my plugins doesn't fail

neat radish
#

But I do appreciate your testing here, and I am hoping that we are near the end here.

solemn ferry
river flame
#

same, just got my code working on the new version

#

everything seems to be golden

#

sharing brigadier command trees between velocity & paper :)

bright leaf
#

I just tested the jar on a copy of our production server, and every existing command seems to work perfectly. Even my somewhat unintended way of manually registering some commands to the command map still works. (Which i didn't expect, but wouldn't care since i will replace it with this api anyway). So in our use case it is a drop in replacement with everything still working as expected. Great work, looking forward to using this API 👍🏻✌🏻

ebon hill
#

that is very valuable, maintaining existing commands exactly how they are is important, especially with all the variety of things people do around commands 😛

solemn ferry
#

@neat radish Why do brigadier cmds in ur new api get registered via the VanillaCommandWrapper and not another wrapper or a modified bukkit one?

neat radish
solemn ferry
#

In the PR, it says in the description that it uses the VanillaCommandWrapper for registration of brigadier commands.

"Deprecates CommandMap, moving all logic to a brig-backed map for legacy support. (All custom brig commands are wrapped in a vanilla command wrapper)"

#

I was curious why it does it in the Vanilla cmd wrapper

neat radish
#

so u can interact with brig commands with bukkit Commands

solemn ferry
neat radish
#

Prolly just cause I wanted to use a new one

solemn ferry
#

alrighty

solemn ferry
neat radish
#

I mean none of that is exposed through the bukkit command api

solemn ferry
viral rock
#

give me brigadier 🙏

rough rose
#

When this pull request is merged, in less than 10 minutes I will remove support for all my plugins to versions older than the latest and use the new command registration structure 👀

granite hound
#

4 minutes left?

peak charm
#

I think they meant it would take 10 minutes from merged to dropping support in their plugins, not 10 minutes to merge

rough rose
#

exactly 😅

granite hound
#

oh

#

the english language can be very confusing

#

even as someone who has only ever known english

ebon hill
#

"when this pull request is merged, in less than 10 minutes ..." vs "when this pull request is merged in less than 10 minutes, ..."

river flame
proud timber
#

Okay I'm bored so I'll go through the pain of building it locally again and testing it in my mod

proud timber
#

Amazing I was able to build it in just over half an hour

proud timber
#

Everything seems to work!

#

One question though 🤔

#

Should a player without access to a command be able to see the help info?

glass smelt
#

generally, no

proud timber
#

Then either I did something wrong or that needs to be fixed

#

Also I really like the new way of registering commands

#

So much cleaner

# 
- this.getServer().getPluginManager().registerEvents(new Listener() {
-   @EventHandler
-   public void load(ServerResourcesLoadEvent event) {
-     event.getCommands().register(BetterConfig.this, ConfigCommand.build());
-   }
- }, this);
+ this.getLifecycleManager().registerEventHandler(LifecycleEvents.COMMANDS, event -> event.registrar().register(ConfigCommand.build()));
static owl
# proud timber

General question: Is this system supposed to register namespaces that contain uppercase letters? Seems unnatural to me since every other namespace in Minecraft can't contain uppercase letters if I remember correctly.

proud timber
#

I mentioned this issue as well

static owl
#

Oh, good 👍

proud timber
#

Although Owen seems to have removed the to do list

#

TODO

  • [ ] Should LiteralArgumentBuilder be accepted in register methods?
  • [ ] Best way to handle argument types with generics? Should we have static getXXX methods on VanillaArgumentTypes for all types?
  • [ ] Handle nullability of "resolved" arguments as it may not be consistent across all of them
  • [ ] Block registration via Commands outside of the event context (will be handled by lifecycle event system)
  • [ ] Move events we want from papermojang-api
  • [x] Make "primitive" vs "native" consistent across the API. Primitive I think doesn't convey that we usually mean all argument types native to minecraft
  • [ ] https://github.com/PaperMC/Paper/pull/9629
  • [ ] command-namespace option for paper-plugin.yml to configure your plugin's default namespace for commands
#

Last option would allow it to be lower case

#

But technically commands aren't namespaced

static owl
#

No, I meant rather to take the plugin name as namespace (which is fine) but since that can contain uppercase letters, register the plugin name with a toLowerCase() call

proud timber
#

So no namespace is uppercase

static owl
glass smelt
#

command names should be lowercased

#

so, if that actually did register a bad command that would be a bug

proud timber
glass smelt
#

thouh, they're not like, real namespaces, as already said

#

though, upper case afaik is fine

#

or, well, actually, I think that there is a weir bunch of coercion logic around in registering stuff

static owl
proud timber
#

But @neat radish why did you delete the todo list?

static owl
proud timber
#

Definitely

glass smelt
#

I'd imagine because it's generally considered close to merging

proud timber
#

In response to why it was deleted?

#

But yeah instead of a config, we could indeed also just call toLowerCase for every command

#

I don't think that breaks anything

neat radish
#

@proud timber Will look into /help thing

proud timber
#

Where did ArgumentTypes.blockPredicate() go?

#

Or did it never exist

#

I thought it did?

proud timber
#

I guess I am mistaken

#

But it would be a crucial addition because it allows for a near perfect BlockArgumentType

#

A type that is very common I'd say

proud timber
#

It is possible to convert between vanilla and bukkit blocks right? Similar to what is being done for BlockState

#

Oh I suppose it's impossible to convert between predicates of those types

#

Is that the reason that argument type isn't added?

proud timber
#

On a second note

#

I suppose ArgumentTypes.blockState reflects the BlockStateArgument in vanilla

#

But those are different things as I just learnt

#

So how does that work?

glass smelt
#

What?

proud timber
#

BlockData is what mojang calls BlockState right

#

Shouldn't it be ArgumentTypes.blockData then?

glass smelt
#

mojang calls it blockState

proud timber
#

Yes

glass smelt
#

I fail to understand why it would make sense to defer the name of the argument type to conform to bukkits misnaming of stuff vs the defacto proper name for the actual arg, assuming that that is using BD on the bukkit side

proud timber
#

The name doesn't matter ofc, but the type of the argument is BlockState

#

Which mismatches with what it is in Spigot, BlockData

#

Hence the type of the argument type should be changed, no?

glass smelt
#

oh

#

No, it doesn't match to BlockData

proud timber
#

But you just said it did

glass smelt
#

Yes, but mojangs side of that arg is not a BlockState

#

it's a BlockInput, which stores the BlockState and the serialised NBT tag of a TE there

#

so, that is analogous to bukkits BlockSate

proud timber
#

I see

#

Mojang being weird

#

Then

glass smelt
#

public class BlockStateArgument implements ArgumentType<BlockInput> {

proud timber
#

Right

#

Strange

#

But yeah then it's good

proud timber
#

In CustomArgumentType, is there any reason for the usage of ArgumentType.super as opposed to this.nativeVanillaType

# 
public final Collection<String> getExamples() {
    return this.nativeVanillaType.getExamples();
}

public <S> CompletableFuture<Suggestions> listSuggestions(CommandContext<S> context, SuggestionsBuilder builder) {
    return ArgumentType.super.listSuggestions(context, builder);
}
proud timber
#

Also

#

Would it be possible to expose a method to create a NamespacedKey from a StringReader?

#

Can do it myself ofc but that's inconvenient

glass smelt
#

Why a StringReader?

proud timber
#

Cause that's what you get in the parse method

glass smelt
#

Was more wondering as there should already be a native adapter for that?

proud timber
#

There is

#

But I can imagine situations where you don't want that?

glass smelt
#

Was more just wonering the utility vs the built in things as-is

proud timber
#

And what do you conclude?

glass smelt
#

No idea

proud timber
#

Lol

#

It can't hurt

glass smelt
#

Yea, I just was wondering why the built in wasn't enough, I generally dislike tossing around API which ends up in weird places and such when theres better options already

proud timber
#

Yeah guess you're right

#

Can't think of a use case on the spot

#

I mean in cases where you absolutely need the StringReader itself but those are rare

#

Btw

#

I tried to implement the custom argument type in Fabric

#

And all I had to do was set the suggestions provider to the custom provider and set the type back to vanilla

#

Basically this

image.png
glass smelt
#

Yes

proud timber
#

It seems to just work

glass smelt
#

same as what paper would do if we support that

proud timber
#

But your implementation is much more involved

glass smelt
#

Well, we have an abstraction layer to deal with

#

I plan to try to review the code soon, was hoping to do it yester-today, but I, er, 👁️

proud timber
#

That's all I am doing

proud timber
glass smelt
#

I've not seen what we do, be we need to provide the frameworking to let people shove in a custom type and get back out something useful

proud timber
#

I have, it's quite involved

proud timber
glass smelt
#

We don't expose the raw tree nd all of it's glory

#

we need to shove a condom over it because API layer

proud timber
#

Mmm okay

#

Interesting word choice, btw

glass smelt
#

When you deal with fabric, you don't need that layer, and so just converting types on the assertion that you can parse it back as i doesn't exactly matter what you see is one thing

#

I, brain, words, AvEisms

proud timber
#

Since my WrappedArgumentType is very much like your CustomArgumentType, what would be the best way to credit it?

#

GPL requires my code to be open source to those who use it, which is satisfied, but how would I give proper credit

#

Oh I suppose CustomArgumentType is licenced under MIT

#

Good enough (at the top of the file)?```
/*

  • Copyright (c) 2024 Owen1212055
  • Permission is hereby granted, free of charge, to any person obtaining a copy
  • of this software and associated documentation files (the "Software"), to deal
  • in the Software without restriction, including without limitation the rights
  • to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
  • copies of the Software, and to permit persons to whom the Software is
  • furnished to do so, subject to the following conditions:
  • The above copyright notice and this permission notice shall be included in all
  • copies or substantial portions of the Software.
  • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
  • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
  • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
  • AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
  • LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
  • OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  • SOFTWARE.
    */
glass smelt
#

I would imagine so

proud timber
#

@neat radish what do you say

neat radish
#

@proud timber idc about legal stuff lol

#

But also brig stuff ours is more involved because we are dealing with the server, you can do whatever you want with the client we cannot

proud timber
proud timber
#

On the client you don't need to bother with converting them at all

neat radish
#

Okay well basically that whole thing mimics one argument type on the server and another on the client

neat radish
proud timber
proud timber
#

Idk if this is related to the Brig API, but you get some mismatches with the following argument type: ```java
public class MaterialArgumentType extends CustomArgumentType.Converted<Material, BlockState> {

private static final Set<NamespacedKey> MATERIALS = Arrays.stream(Material.values())
    .filter(Material::isBlock)
    .map(Material::getKey)
    .collect(Collectors.toUnmodifiableSet());

private MaterialArgumentType() {
    super(ArgumentTypes.blockState());
}

public static MaterialArgumentType material() {
    return new MaterialArgumentType();
}

@Override
public @NotNull Material convert(@NotNull BlockState state) {
    return state.getType();
}

@Override
public @NotNull <S> CompletableFuture<Suggestions> listSuggestions(@NotNull CommandContext<S> context, @NotNull SuggestionsBuilder builder) {
    return SuggestionProviderHelper.suggestNamespacedKeys(MATERIALS, builder);
}

}

#

All mismatches: minecraft:chiseled_copper, minecraft:chiseled_tuff, minecraft:chiseled_tuff_bricks, minecraft:copper_trapdoor, minecraft:crafter, minecraft:exposed_copper_bulb, minecraft:exposed_copper_door, minecraft:exposed_copper_grate, minecraft:exposed_copper_trapdoor, minecraft:oxidized:copper_bulb, minecraft:oxidized:copper_door, minecraft:oxidized:copper_grate, minecraft:oxidized:copper_trapdoor, minecraft:polished_tuff, minecraft:polished_tuff_slab, minecraft:polished_tuff_stairs, minecraft:polished_tuff_wall, minecraft:trial_spawner, minecraft:tuff_brick_slab, minecraft:tuff_brick_stairs, minecraft:tuff_brick_wall, minecraft:tuff_bricks, minecraft:tuff_slab, minecraft:tuff_stairs, minecraft:tuff_wall, minecraft:waxed_exposed_chiseled_copper, minecraft:waxed_exposed_copper_bulb, minecraft:waxed_exposed_copper_door, minecraft:waxed_exposed_copper_grate, minecraft:waxed_exposed_copper_trapdoor, minecraft:waxed_oxidized_chiseled_copper, minecraft:waxed_oxidized_copper_bulb, minecraft:waxed_oxidized_copper_door, minecraft:waxed_oxidized_copper_grate, minecraft:waxed_oxidized_copper_trapdoor, minecraft:waxed_weathered_chiseled_copper, minecraft:waxed_weathered_copper_bulb, minecraft:waxed_weathered_copper_door, minecraft:waxed_weathered_copper_grate, minecraft:waxed_weathered_copper_trapdoor, minecraft:weathered_chiseled_copper, minecraft:weathered_copper_bulb, minecraft:weathered_copper_door, minecraft:weathered_copper_grate, minecraft:weathered_copper_trapdoor

neat radish
proud timber
#

Experimental as in?

ebon hill
#

minecraft experimental

#

you need the 1.21 experimental datapack to enable the 1.21 experimental features

proud timber
#

Oh

#

Perhaps Fabric automatically enables those

#

Don't recall doing that

proud timber
#

Wait huh

#

The suggestions are from the Material class

#

Oh they are in there already

#

That's annoying

#

Do I really need to use reflection to get only the ones that aren't experimental

glass smelt
#

Material#isEnabledByFeature

neat radish
#

You have to enable the experimental flags..

proud timber
proud timber
neat radish
#

Then don’t enable the flags? This is a non issue

neat radish
#

Pass null, or something

glass smelt
#

needs a world due to how mojang implemented the check

proud timber
#

Yeah

#

World is marked as @NotNull

glass smelt
#

it's not even tied to the world internally

#

Mojang is in this weird area where it looks like some stuff is or could be per world, but then they end up just passing in global state from elsewhere to back it

#

it's, weird

proud timber
#

You do enable the experiments in the create world screen

#

But for a dedicated server you set them in server.properties

glass smelt
#

I mean, but that's not a world

#

that's a save

#

Mojangs terminology here sucks ass

#

Doesn't help that the way CB handles worlds is the code I've been exposed to and is hell

proud timber
#

Can't Paper make an API that takes the server instance instead of the world

glass smelt
#

Yes

proud timber
#

But it just isn't there yet?

#

Don't tell me that's a yes it can't lmao

glass smelt
#

But, yes, such a thing could be added

#

it taking a world is kinda dumb, then again, mojang could wake up tomorrow and limit stuff per dimension

proud timber
#

Yeah right

#

But for now it means you can't really make the argument type work unless you enable experimental features

#

Reflection was also not possible as I noticed, both annotations are removed at run time

neat radish
#

spigot's feature flag api sucks tho

proud timber
glass smelt
#

depends on how deep the concept goes

proud timber
#

Blocks wouldn't work ofc

glass smelt
#

Yea, ik what it is

#

Like

#

This is one of those detatchments between upstream and mojang on how stuff is handled

#

Like, in retrospect what I said was dumb because I forgot about that caveat

gusty iris
#

Mojang could allow this at any time, since it's already checked using the world and not the global state (just right now the world redirects this to global state)

proud timber
#

You can transfer blocks between dimensions yk

glass smelt
#

The confusion is how spigot, or, well, craftbukkit abuses ServerLevel

#

the way worlds works is really weird, and so it's not an issue

#

there is pretty much 0 reason why it needs to take a world, as that's not how this thing works

proud timber
#

Yeah must say I'm starting to doubt whether I know how this works

#

Retaining @MinecraftExperimental at runtime would provide a quick (hacky) solution to this I suppose

proud timber
# 
private static final DynamicCommandExceptionType INVALID_STRUCTURE_ID_EXCEPTION = new DynamicCommandExceptionType(id -> MessageComponentSerializer.message().serialize(Component.translatable("structure_block.invalid_structure_name", id.toString())));
```is giving me `Invalid structure name '%s'` when I do `throw INVALID_STRUCTURE_ID_EXCEPTION.create(NamespacedKey key)`
#

Yes I am abusing a structure block translation key for structures, call me smart

proud timber
#

Any idea?

modern pagoda
#

i dont want to read the entire history of this discussion
what things are still to be tested?

#

and what is the end goal?
replace the command map with the command dispatcher ?