Long chains of optional arguments = lots of code duplicated about brigadier HOT 10 OPEN

Honestly a more "proper" API, like what android uses, would be a much simpler method for us end-users than this lazy fallback to abusing java 8 functionality. Not to mention it could be documented in a more organized fashion, and it would be substantially more performance friendly (you remember, that thing the java version hasn't been since 1.7 because someone acquired a parsing fetish?)

Additionally such a system would make it far easier to port existing forge mods, rather than making us have to rewrite all that stuff AGAIN.

from brigadier.

Hello, in the original post I said I opened this issue for discussion, not whatever this is.
Arguing over whether lambdas should even be used for example is completely off topic and doesn't help anyone decide what to do about optional arguments.

from brigadier.

You know what. You're completely right.
The entire design as a whole being inconvenient and just bad practice in every possible way, has nothing at all to do with the fact it creates the limitation issue you are experiencing, or that a proper design wouldn't have that issue in the first place.
My bad, this is completely unrelated.

from brigadier.

Saying "the API is bad. Make a new API." is not in any way specific or constructive.
I aimed at a very specific API design, for a very specific reason, I'm sorry I don't take 20 paragraphs to essay out a simple point. Here, let me do that this time.

Lambdas have the same performance behavior as anonymous classes, if I recall correctly (I haven't looked at the specifics, but it's effectively a shorthand for an inner class except done in a nicer way at runtime).
At runtime, is exactly the issue, pre-compiled is ALWAYS more performance friendly than doing things at runtime, not just because everything that happens at runtime has to be stored, unlike pre-compiled that can just be called at convenience. Of course there are times when having to do it on runtime is the only option, but this is not one of those.
Additionally at runtime is a lot more error prone and it's harder for IDE's to detect errors before compiling, unlike pre-compiled where it can usually see what you've screwed up before you press build. It's the same reason the forge devs used to advise against ASM, before it became cool and changed names to this mess.

If you want to do things in your so-called proper way, you totally can. It'd just be messy, and without any real benefit.
yeah you're right, that example code is messy and without benifit, because it's not actually the proper way. It's the same garbage, worded a little differently to look the proper way, any idiot who's actually worked in java 7 and older would see the difference.
A proper way would be creating a class that extends an existing one, doing a simple @Override of the function you need to modify, then putting an instance of the class into a registry or as a variable for another function to "spawn" it, for lack of a better term off the top of my head.
The end result could be all of 6 lines of code, including whitespace package name and imports, while being even more legible.

If you wanted to do this, you could register a single string argument and parse everything yourself.
There's that word again. Parse. Regex has to have it's way with everything, so all data whether in use or not has to be processed, stored, loaded and processed again, rather than just processed once.
And the playerbase wonders why a game that's less complex than a AAA title from windows 98 runs like garbage on an i3? The playerbase is already suffering for reasons a bit more dramatic than an inconvenient interface.
Insult to injury your example code isn't designed anything like the average forge mod.

If you're still using a 4-year-old version, then this library really does nothing for you. And all of those changes had a benefit of extensibility via resource packs at the cost of performance, which in general is the goal of a ton of the past updates
It was an example to elaborate a point, I'm working significantly on some 1.12 mods currently but I maintain the older versions still because people still use them (sorry, I realize supporting users preferred versions of the game is a foreign concept to you people).
Although I'm sure you realize this was just an example, and you're just taking it out of context to push your own point. Or maybe I'm giving you too much credit on that, who knows.

Ah, you're one of those people. Probably shouldn't have bothered writing this up, then. But it's already written, so oh well.
To be honest I'm getting the same feeling. I made my comments in hopes of some REAL developers wanting to make some REAL change to a game I and most of my friends enjoy playing and developing for.
For as long as most of us (referring to my friends and I) have been around, this new API is just insulting.
"In the name of extensibility" my bunghole, it's no more or less extensible than minecraft 1.6 was, you people just changed the ways it's done to be less efficient, more complex, more tedious to write, and overall harder to maintain from both the API's end and the modder's end.
To be quite honest I'm more or less at the end of my rope for any hope, as you all just seem to be a bunch of lazy amateurs who want everything to run the "cool" way while having your work spoon-fed to you, because you can't be bothered to write real code or actually care about the playerbase.
And you're all clearly very content with the shameful state of this game that windows 98 would laugh at taking more than a gig of ram and a processor that makes windows vista look dated to run vaguely smoothly.

Microsoft buying the game was quite fitting, windows 10 took 30+ times more resources to run than XP for generally the same features, plus graphics that DOS would laugh at. But in exchange for the fancy new API's that basically did the same as the old ones, but in parseable methods that were "cooler" to write. Why not minecraft 1.14? Crappy lazily written API's for all. Micro$hafted again.
And to think I called all the people that quit the scene in 1.8 "crazy" because they saw this coming. I guess they were right after all.

So I'm just gonna end my point on that rant and leave, in HOPES, someone with some actual skill, passion, and familiarity with the scene wants to come around and tell me there is actually a REAL API in the works, because I'm about done with you kids circlejerking eachother trying to look cool rather than be concerned about product quality and/or the actual userbase.

from brigadier.

@ryantheleach

1. Google isn't hard to use, you can literally just search their API documentation and get a good idea of how they use simple java overrides, its quick and simple to use in small segments of code if you know what to tie into, like older forge, or do you need to google that too?

2. Just because it's done often doesn't mean it's good practice, or convenient to the end-users. If they wanna do it the stupid way they should have the choice, but for those of us who actually know what we're doing and have some degree of concern for the low-end users, it would be good to have PROPER methods.

3. It does relate to the issue as the change would allow for that to happen far more efficiently. Additionally I would assume most here have played older minecraft? I have a community of over 10k users that STILL play 1.7, so it's not exactly a lost art, plenty of people remember it and the performance falls that happened with all the .json models and configs.
   And again, google exists for a reason.

But thanks for wasting my time trying to "correct" me with all those "facts" or whatever it is you tried and failed to do. Now back to your SJW hole.

Also while I'm on the rant I'd like to say "thanks" to the 8 people who thumbs downed me. Really showing the "quality" of the community.

from brigadier.

Saying "the API is bad. Make a new API." is not in any way specific or constructive.

Just because it's done often doesn't mean it's good practice, or convenient to the end-users. If they wanna do it the stupid way they should have the choice, but for those of us who actually know what we're doing and have some degree of concern for the low-end users, it would be good to have PROPER methods.

Lambdas have the same performance behavior as anonymous classes, if I recall correctly (I haven't looked at the specifics, but it's effectively a shorthand for an inner class except done in a nicer way at runtime). If you want to do things in your so-called proper way, you totally can. It'd just be messy, and without any real benefit.

Something like this:

Predicate<CommandSourceStack> pred = new Predicate<>() {
    @Override
    public boolean test(CommandSourceStack source) {
        return true;
    }
}
LiteralCommandNode<CommandSourceStack> literal = new LiteralCommandNode<>("foo", new Command<>() {
    @Override
    public int run(CommandContext<CommandSourceStack> context) {
        System.out.println("Called foo with no arguments");
        return 1;
    }
}, pred, null, null, false);
literal.addChild(new ArgumentCommandNode<>("bar", IntegerArgumentType.integer(), new Command<>() {
    @Override
    public int run(CommandContext<CommandSourceStack> context) {
        System.out.println("Bar is " + IntegerArgumentTypegetInteger(context, "bar"));
        return 1;
    }
}, pred, null, null, false, null);
dispatcher.register(literal);

Granted, it isn't obvious that you can do that due to the lack of documentation, but that still isn't this issue (and this issue would still be present with that method of doing things, I think).

EDIT

And to add to that, from your original comment:

Additionally such a system would make it far easier to port existing forge mods, rather than making us have to rewrite all that stuff AGAIN.

If you wanted to do this, you could register a single string argument and parse everything yourself. You'd not get any benefits of having arguments displayed in tab-completion (so your users would suffer), but you could do it for the purpose of porting.

END EDIT

I have a community of over 10k users that STILL play 1.7, so it's not exactly a lost art, plenty of people remember it and the performance falls that happened with all the .json models and configs.

If you're still using a 4-year-old version, then this library really does nothing for you. And all of those changes had a benefit of extensibility via resource packs at the cost of performance, which in general is the goal of a ton of the past updates. However, debating this is definitely off-topic, so I'll stop here.

Now back to your SJW hole.

Ah, you're one of those people. Probably shouldn't have bothered writing this up, then. But it's already written, so oh well.

from brigadier.

Not unrelated but still off-topic in this thread. The fact it is related to this issue, does not mean you should discuss it here.

This thread is only about this specific inconvenience and people don't want to read a lengthy discussion here about something that goes far beyond this ticket (which is btw why I downvoted your comments and others).

If you want to discuss the implementation of this library, I suggest you start a new thread.
Of course, you can mention this ticket and others, because, as you said, it is related.

from brigadier.

@EternalBlueFlame I'll bite.

1. You assume people have knowledge of Android.
2. "lazy fallback to abusing java 8 functionality" Commands in various programming languages are often implemented using Lambda's / Closures.
3. "Not to mention it could be documented in a more organized fashion, and it would be substantially more performance friendly (you remember, that thing the java version hasn't been since 1.7 because someone acquired a parsing fetish?)" Without examples, people arn't going to be sure about what you are on about. They are aware of a lack of documentation (#35) and it doesn't relate to this issue at all.

from brigadier.

Why do the static methods like getString(final CommandContext<?> context, final String name) even exist? CommandContext's getArgument is generic and works perfectly fine. I'd argue that these static methods are already overhead.
Without them, you could add the orDefault method easily to the CommandContext class without creating any additional overhead.

from brigadier.

Okay, I came up with a different concept that allows you basically to do what you want and is, imo, even a bit neater.
I did not make it a pull request yet, in order to avoid creating a new pull request that's gonna be rejected for similar reasons. Also, I was to lazy to add tests yet.

You can find my concept in my fork of the project: fork / Changes

It introduces default arguments. Every CommandNode can have one default node. Whenever there's no more input to parse, it will use the default node. In case of an ArgumentCommandNode a default value needs to be specified. This default value will be added automatically to the CommandContext, when there's nothing left to parse in the StringReader.

A basic example would be the following:

dispatcher.register(literal("foo")
    .then(defaultLiteral("bar")
        .executes(ctx -> {
            System.out.println("bar");
            return 0;
        }))
    .then(literal("baz")
        .executes(ctx -> {
            System.out.println("baz");
            return 0;
        })
    )
);

The command foo has the child literal arguments bar and baz. bar is in this case the default, since it was created with defaultLiteral() rather than literal().
foo will print bar, foo bar as well and foo baz prints baz

dispatcher.register(literal("foo")
    .then(defaultArgument("bar", integer(), 42)
        .executes(ctx -> {
            System.out.println(ctx.getArgument("bar", Integer.class));
            return 0;
        }))
    .then(literal("baz")
        .executes(ctx -> {
            System.out.println("baz");
            return 0;
        })
    )
);

This is very similar to the first example, except it uses an integer argument. foo will print 42, and foo 5 5.

dispatcher.register(literal("foo")
    .then(defaultArgument("bar", integer(), 42)
        .then(defaultArgument("baz", word(), "abc")
            .executes(ctx -> {
                System.out.println(ctx.getArgument("bar", Integer.class) + ", " + ctx.getArgument("baz", String.class));
                return 0;
            })
        )
    )
);

Of course, default commands can be chained. foo in this case would print 42, abc. foo abc would be invalid, since an integer is expected on the 2nd position. You cannot simply ignore default arguments. foo 5 def works as you'd expect it to.

dispatcher.register(literal("foo")
    .then(defaultArgument("bar", integer(), 42)
        .then(argument("baz", word())
            .executes(ctx -> {
                System.out.println(ctx.getArgument("bar", Integer.class) + ", " + ctx.getArgument("baz", String.class));
                return 0;
            })
        )
    )
);

This is an interesting construct, because there's a default argument followed by a non-default. Since bar is not executable, you have to specify every argument. If bar had an executes-clause as well, it'd get executed with the input foo.

Sanity checks are not implemented (yet?). E.g. defaultArgument("bar", integer(0, 10), 42) is not prevented and might add 42 to the command context. Preventing that would probably mean to blow up the ArgumentType classes again by adding a isValidValue method to them.
Adding a defaultLiteral to the root is possible, but an empty command string will not use that command by default. Probably should throw an IllegalArgumentException or something.
Trying to add multiple default nodes to a single node will throw an IllegalStateException.

from brigadier.