Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Suggestion] Optional Arguments #110

Open
Speiger opened this issue Apr 30, 2022 · 6 comments
Open

[Suggestion] Optional Arguments #110

Speiger opened this issue Apr 30, 2022 · 6 comments

Comments

@Speiger
Copy link

Speiger commented Apr 30, 2022

Ok this is a bit rough to start.

When commands in Minecraft are created, you have either 2 ways of creating commands that are executed.
Either implement every single execution itself. (If you have 5 optional parameters that's getting annoying really quickly)
Or you implement a CommandContext Wrapper that basically does this:

	public boolean hasValue(String id, Class<?> type)
	{
		try
		{
			return source.getArgument(id, type) != null;
		}
		catch(Exception e)
		{
			return false;
		}
	}
	
	public <T> T getOrDefault(String id, Class<T> type, T defaultValue)
	{
		try
		{
			return source.getArgument(id, type);
		}
		catch(Exception e)
		{
		}
		return defaultValue;
	}

It would be nice if you could check if arguments have been defined or allow to getOrDefault.
This is mainly there to reduce code. Implementing everything 5 times. Even if Lambdas are being used that call a helper function that basically does the same.
Instead this could look a lot cleaner.

Here is a example with my CommandBuilder & Command Wrapper

	public static CommandBuilder createGenStart()
	{
		CommandBuilder builder = new CommandBuilder("gen");
		//Normal Gen
		Command<CommandSource> radius = GenCommand::executeRadius;
		builder.addLiteral("radius");
		builder.addArgument("Task Name", StringArgumentType.word());
		builder.addArgument("Shape", StringArgumentType.word(), GenCommand::listShape);
		builder.addArgument("Center", ColumnPosArgument.columnPos(), CenterArgument::listSimpleSuggestion);
		builder.addArgument("Radius", IntegerArgumentType.integer(1, 25000), radius);
		builder.addArgument("Dimension", DimensionArgument.getDimension(), radius); //Optional
		builder.addArgument("Generation Type", StringArgumentType.word(), GenCommand::listSuggestions, radius).popTop(); //PopTop basically goes down the logic tree allowing to make a new branch. A single/multi pop exists too.
		
		Command<CommandSource> expansion = GenCommand::executeExpansion;
		builder.addLiteral("expansion");
		builder.addArgument("Task Name", StringArgumentType.word());
		builder.addArgument("Shape", StringArgumentType.word(), GenCommand::listShape);
		builder.addArgument("Center", ColumnPosArgument.columnPos(), CenterArgument::listSimpleSuggestion);
		builder.addArgument("Min Radius", IntegerArgumentType.integer(1));
		builder.addArgument("Max Radius", IntegerArgumentType.integer(1), expansion);
		builder.addArgument("Dimension", DimensionArgument.getDimension(), expansion);
		builder.addArgument("Generation Type", StringArgumentType.word(), GenCommand::listSuggestions, expansion).popTop();
		return builder;
	}
	
	private static int executeRadius(CommandContext<CommandSource> source)
	{
		CommandWrapper wrapper = new CommandWrapper(source);
		String name = wrapper.get("Task Name", String.class);
		GenShape shape = wrapper.hasValue("Shape", String.class) ? GenShape.valueOf(wrapper.get("Shape", String.class)) : wrapper.get("Shape", GenShape.class);
		BlockPos center = CenterArgument.getVanillaBlockPos(wrapper.get("Center", ILocationArgument.class), source.getSource());
		int radius = wrapper.get("Radius", Integer.class);
		ResourceLocation dimension = wrapper.getOrDefault("Dimension", ResourceLocation.class, wrapper.getSource().getWorld().getDimensionKey().getLocation());
		return 0;
	}

The Reason for hasValue is used for the GenShape is a example. What if the software that the CommandContext uses only exists on 1 side. It can detect if a single side exists, or if dual sided implementation is present.
Or in modding terms: ServerOnly Mods.

This is how I would implement it.
If you want I can make it a offical PR, or you can just grab the implementation.
Reference Code: https://github.com/Mojang/brigadier/blob/master/src/main/java/com/mojang/brigadier/context/CommandContext.java#L81

    public <V> boolean hasArgument(final String name, final Class<V> clazz) {
        final ParsedArgument<S, ?> argument = arguments.get(name);

        if (argument == null) {
            return false;
        }
        final Object result = argument.getResult();
        if (PRIMITIVE_TO_WRAPPER.getOrDefault(clazz, clazz).isAssignableFrom(result.getClass())) {
            return true;
        } else {
            return false;
        }
    }
	
    public <V> V getArgument(final String name, final Class<V> clazz, V defaultValue) {
        final ParsedArgument<S, ?> argument = arguments.get(name);

        if (argument == null) {
            return defaultValue;
        }

        final Object result = argument.getResult();
        if (PRIMITIVE_TO_WRAPPER.getOrDefault(clazz, clazz).isAssignableFrom(result.getClass())) {
            return (V) result;
        } else {
            return defaultValue;
        }
    }
@Gamebuster19901
Copy link

Are redirects not a solution for this?

@Speiger
Copy link
Author

Speiger commented Jun 14, 2023

@Gamebuster19901 the point is: You are required to write duplicated implementations for every single Optional argument.
Or write a specific handler that redirects to a function where you insert the default argument.
Causing for a LOT of code duplication. 1 function where you handle everything, including Default Arguments is a lot simpler to deal with and also a lot easier to read.

Just look at my code example that I have done and replicate it and look how much worse it looks and is to maintain if you don't have these features.

I dunno what you think but if you have 8 versions to maintain and have to do fixes for one or two functions that have to interact with that it just gets bad.
This is why i have added a wrapper for myself that allows for this kind of functionality, to make this possible.

And it is quite sad that Mojang doesn't see the Benefit of such a function that doesn't have ANY downsides.

@KitsuneAlex
Copy link

Giving this a friendly bump because this would be lovely

@LoboMetalurgico
Copy link

Yes, please! We need this!

@M4ximumPizza
Copy link

This would make this type of development significantly faster.

@Speiger
Copy link
Author

Speiger commented Nov 6, 2023

Honestly this is just a very tiny QoL change that is "Optional" yet mojang hasn't included.
I don't expect Mojang to ever include this.

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

No branches or pull requests

5 participants