tterrag09/08/2019
there's literally no way to support vanilla clients
if you add serializers, vanilla clients break, if you don't forge clients break
they HAVE to match between server and client even if IT SENDS NO DATA
we'll have to implement some kind of client-aware sync where it doesn't send modded command nodes to vanilla clients
this might be beyond what I can do tonight
because really I have no idea where to start
how would you even detect a vanilla client at this point?
LexManos09/08/2019
..
Or we just nuke the custom argument
and parse it yourself later
tterrag09/08/2019
ugh
but this means any mods that want to be server only can't add custom argument types?
LexManos09/08/2019
You should be able to still give suggestions
tterrag09/08/2019
no wonder plugins in 1.13+ are a fuckin mess
LexManos09/08/2019
Also
why has this not been a issue]
for commands Forge adds using the two argumetns we already have?
ModID/EnumArgument
tterrag09/08/2019
they were removed
at least, the serializers were
not sure how those don't crash tbh
LexManos09/08/2019
No the serializer for enum wass removed
because it required extra data
tterrag09/08/2019
oh you know what
it's because they were at the end of the data, so it probably didn't matter
my argument gets smack in the middle and the buffer offset gets fucked
that's my only guess anyways
LexManos09/08/2019
... god dammet im gunna have to dig through this myself aint i...
tterrag09/08/2019
don't bother
I'll just move it to a string argument and move on
LexManos09/08/2019
What does it loose functionality wise?
tterrag09/08/2019
wait, it can't be a string argument because strings apparently can't have colons
because MINECRAFT
LexManos09/08/2019
thejn how the fuck does it work now?
tterrag09/08/2019
it doesn't work now, that's why we're here?
LexManos09/08/2019
No
tterrag09/08/2019
I added a whole custom parser to handle the weird backwards compat
LexManos09/08/2019
the vanilla command
tterrag09/08/2019
it's all hardcoded shit
just strings
"Village"
"Mansion"
literals
LexManos09/08/2019
humm
tterrag09/08/2019
so either we disallow namespaces on structures (bad idea)
or...idk
LexManos09/08/2019
running a setup
gunna see what explodes
tterrag09/08/2019
the client data reader is just really dumb here. if it comes across an argument it doesn't know it just says "oh well we'll skip it" except it doesn't read out the buffer data that's there
this needs to be fixed for 1.15
LexManos09/08/2019
'the buffer data thats there'?
Thats where?
tterrag09/08/2019
put a sentinel between each argument type and flush the remaining unknown data
ok so
when an argument is serialized there's 3 parts
the ID, the data added by the serializer, and the suggestion data
LexManos09/08/2019
But yes, they DO need to add a data size to serialized arguments
oh
what is the format of the packet?
tterrag09/08/2019
on the client, it reads the ID, sees no type registered, then goes "welp nothing I can do" leaving the suggestion data in the buffer
then the next argument starts reading garbage from the suggestion data and everything is fucked
exactly? um
LexManos09/08/2019
which packet is it?
tterrag09/08/2019
byte: flags
varint: child_count
varint[child_count]: children
varint: redirect
string: name
blob: argument serializer data
rl: suggestion provider
that for each argument type
SCommandListPacket
so what gets undefrlowed is the suggestion provider
so the other two arguments are fine because they don't have one
there is no .suggests
which is lame but probably done to avoid this issue, or just out of laziness
but either way it sidesteps the problem. but I cannot leave out the suggestions here, have to keep vanilla parity
the client literally just needs to read the suggestion data regardless (even if just to throw it away)
even if the argument type is unknown on the client, whether or not there is a suggestion provider is encoded in the flags (bit 16)
so really that's the oversight, but we can't patch it. the solution would be to leave off suggestion data for modded argument types being sent to vanilla clients. but I have nfi how to even begin to make that happen. and we should tell mojang about this for 1.15 as it pretty much cripples modded serverside commands
LexManos09/08/2019
Right this wouldnt fix vanilla clients
but we could fix forge by moving the read of the suggestions out of the else check
Mojang should do that.
But for us, to keep compat with vanilla
Don't send suggestions
tterrag09/08/2019
have to, because it's a vanilla command
plus it makes the command rather useless
you'd have to randomly guess at structure names
LexManos09/08/2019
register("resource_location", ResourceLocationArgument.class, new ArgumentSerializer<>(ResourceLocationArgument::resourceLocation));
We cant use that?
tterrag09/08/2019
nope, because I have to handle the legacy stupid names, like "Village"
which is not a valid RL and causes it to choke
LexManos09/08/2019
humm
tterrag09/08/2019
unless we're okay with breaking that
it's technically a behavior change, but I'm not sure anyone would care?
LexManos09/08/2019
wait
what if you stuck the new argument
at the end of the .then chain
tterrag09/08/2019
hm
right
LexManos09/08/2019
and let the previous node handle the non-RL format shit
tterrag09/08/2019
I suppose that would work
sidesteps the problem that WILL crop up again, but that's future us's problem
LexManos09/08/2019
By future us, I hope we can convice @fry to fix the network protocol in 1.15+
tterrag09/08/2019
well yeah, I more mean within 1.14, someone either us or a modder is gonna hit this issue again
LexManos09/08/2019
yes they will
and its shitty
what we CAN do is fix the issue on our end
for Forge clients
tterrag09/08/2019
as proof of concept, yeah
LexManos09/08/2019
and then if OTHER people run into it in their mod, we can say fuck it.
Their mod isnt vanilla compat.
Tho.. we SHOULD have some form of standard test for OUR code.
tterrag09/08/2019
I'm gonna write that patch now while I still have this code here
LexManos09/08/2019
if we have a way to test in just ForgeDev mode.
tterrag09/08/2019
thinking we just do this?
         if (argumenttype == null) {
            if ((flags & 16) != 0) {
               buf.readResourceLocation(); // FORGE: Flush unused suggestion data
            }
            return null;
         } else {
I don't think there's a way to recover the suggestion data in a useful way, since it needs the nonnull argument type
LexManos09/08/2019
its more then that
wait
is it literally just a RL for the suggestions type?
tterrag09/08/2019
yup
LexManos09/08/2019
jesus christ
ya that should be a fix
tterrag09/08/2019
and in our case it's always ask_server
let me test it
LexManos09/08/2019
be sure to test it without this fix
to make sure vanilla can still work
tterrag09/08/2019
yeah
I'll just be removing the argument type again, so I don't see why it wouldn't, but I will test a vanilla client
lookie it works perfectly
[image]
no suggestions but the command runs
(patched client, no registered serializer)
LexManos09/08/2019
even with moving the argument?
tterrag09/08/2019
this is without any changes
the current crashing setup
LexManos09/08/2019
ah ya i wouldnt expect any suggestions
tterrag09/08/2019
nope, this is intended behavior
no way to provide suggestions without an argument type
it's flawed system, ideally they should be separated
there's no real reason why suggestions should depend on the type being serializeable
it's just a list of strings from the server
LexManos09/08/2019
ya
Also, ArgumentTypes should have a length header
so that unknown types can just be nuked from the buffer, and treated as a 'string' type.
tterrag09/08/2019
for sure
can't do that in forge though, it changes protocol :/
LexManos09/08/2019
Wanna file a jira bug on this, and i'll yell at Fry to fix?
tterrag09/08/2019
might have to be tomorrow, I'll already be staying up too late to finish this fix :stuck_out_tongue:
LexManos09/08/2019
In theory, falling back the 'String argument, which asks the server for suggestions' should be safe for literally anything.
tterrag09/08/2019
yeah I don't really see why it isn't just done that way in general
LexManos09/08/2019
Ya no rush, just stuff to yell at them about
tterrag09/08/2019
why does the client give a shit about how to serialize the argument?
I guess for metadata purposes
LexManos09/08/2019
Well we dont need to ask the server to suggest a blockpos
tterrag09/08/2019
but "dumb string" is a sensible fallback
I suppose that's true, it reduces lag