[Question] (Scarpet) Would a PR doing this (extended) be accepted?

[altrisi]

I've kinda had nothing to do for a while, and confused myself with what I needed to have for tests in my (yet useless) ScarpetAppTester. I thought that for some reason I needed to know the whole list of functions in Scarpet (I don't, that program if finished will check user-defined functions, not native ones), and then started thinking at ways of improving the current way of registering functions while also keeping them in a list.

Even though I realized I didn't need that at all, I liked the idea I had of annotating public methods and inferring all arguments and things from there. My current idea is to also allow to declare different types in the different arguments of those methods and make the parser automatically convert the values (and handle the errors when incorrect) to ease coding new functions and deduplicate code (mostly casting and instanceofs, also parameter count checking, value unpacking, etc).

I already have quite some base working, but I'd like to know whether the PR with that would be accepted before coding everything (although I enjoyed doing so and may do it anyway at some point). Currently, it can already create fixed and variable argument functions passing Values and waiting for LazyValues. But the idea is to be able to pass specific values, unwrapped values (like Entity, String, Double, Text, etc), or even extra parsed ones (like ServerPlayerEntity), passing the Context if needed (currently the petition is registered but it isn't really passed), accepting Values as return and converting them to LazyValues (since that's actually quite common), and maybe passing LazyValues too if requested instead of evaling every parameter pre-execution. The first four already have some kind-of a ~base~ something started.

Preview of the current functionality (code still a bit messy) (after watching it again, not that much useful to watch):

https://streamable.com/kfbwjl

Example code for defining functions with this:

Currently working:

@LazyFunction(maxParams = 6)
public LazyValue semi_multiparams(Value fixed, Value... values) {
    String str = fixed.getString();
    for (Value val : values)
        str += val.getString();
    Value retval = StringValue.of(str);
    return (c,t) -> retval;
}

Concept for the final version (kind-of reimplementation of display_title):

@LazyFunction(maxParams = 6)
public Boolean display_title(ServerPlayerEntity target, String actionString, Text content, int... times) {
    TitleS2CPacket.Action action;
    switch (actionString.toLowerCase(Locale.ROOT))
    {
        case "title":
            action = Action.TITLE;
            break;
        case "subtitle":
            action = Action.SUBTITLE;
            break;
        case "actionbar":
            action = Action.ACTIONBAR;
            break;
        default:
            throw new InternalExpressionException("'display_title' requires 'title', 'subtitle' or 'actionbar' as second argument");
    }
    if (times.length == 3)
    {
        target.networkHandler.sendPacket(new TitleS2CPacket(Action.TIMES, null, times[0], times[1], times[2]));
    }
    target.networkHandler.sendPacket(new TitleS2CPacket(action, content));
    return true;
}

static modifier isn't really needed, I may just even remove it for the sake of simplicity by requiring to pass an instance of the class on parse. Changed

Now I'm also thinking about allowing Enums in parameters too for predefined options, such as in this case, a custom enum with a variable with the Action itself, with values being the options supported.

[gnembon]

Problem is that first of all this may break compatibility of extensions, if another system is introduced (UNLESS its just a wrapper for more convenient notation - in that case that could be useful in 'general' API calls.

The thing is most functions should be able to be defined in a fun(Value..) -> Value fashion not fun(LazyValue....) -> LazyValue way. Problem was that I initially didn't provide regular functions type Value... -> Value with context (not really needed for most built-in math functions for example), but context is essential in MC Api functions.

Having a wrapper that un-wraps LazyValues could be cool to have (as an alternative), but you have to keep in mind that you would be trading flexibility with interpretting arguments differently (pretty much all functions that take a block value (three ints, or block value, sometimes entity is also ok etc) -> that's why you have some of those functionalities wrapped inside argument locators.

Maybe instead of that provide a universal argument ingester class, like

fun(c, t, i = ingester(c, t, lv)) ->
  ServerPlayerEntity e = i.getPlayer();
  String actionString = i.getString(validate=['title','sibtitile','actionbar'], case_sensitive=false);
  Text what = i.getText();
  List<Integer> = i.getIntTail();

this way client can be more flexible in what it does and which order - this way this can be used in more cases in the API where order of args do change from call to call (pardon my mix of scarpet, java and python notation)

[altrisi]

At least currently yes, this is just a wrapper that scans a given class file, saves processed functions to a ~map (or set, can't really remember, I'm on mobile rn)~ list and then calls addLazyFunction with every registered function on apply().

The idea was to make defining some functions in general simpler by converting some types directly (well, the original idea was really having fun with reflection, but more or less), while also keeping the order of arguments in place, so it could provide auto-generated error messages (eg fun requires a player as third argument), since I find type checking, conversions and argument counting the most repeated pattern when defining a function in Scarpet, and in some functions that's actually skipped (there are some functions that accept unlimited args then ignore any extra ones to its usage, for instance).

In the original code I had I didn't really think in argument locators (yet), but I think it's something that I'd have to have done. Not sure I really understand the ingester though (edit: After reading it again, I feel like I misunderstood it when formulating the following, as I saw it as an extra to lv, not as a replacement): How would it keep order of arguments? Would it mean you have to access it in the correct order to get every of the arguments right (so they are they are kinda consumed), or how would that be?

Anyway I'll see what I end up doing with this. I feel like that part would be a different thing to my idea, since what I had is basically, as you mentioned, an idea for something more convenient to create most API calls that don't really need much complexity (the idea is to make it a bit complex though), while keeping the current one for the rest.

BTW what is t in lazy functions and what is it used for? Should I allow a method to request it being passed? Or does it always need to be when context is passed? (in which case I may just pass both if context is asked) (/is there any place where t would be used without using a Context (except for evalValue, ofc))

[altrisi]

Progress update (in case anyone cares, which I would appreciate):

The following already works, doing exactly* like the current display_title function:

@LazyFunction(maxParams = 6)
public Boolean display_title2(@AllowSingleton List<ServerPlayerEntity> targets, String actionString, Text content, Integer... times)
{
  TitleS2CPacket.Action action;
  switch (actionString.toLowerCase(Locale.ROOT))
  {
      case "title":
          action = Action.TITLE;
          break;
      case "subtitle":
          action = Action.SUBTITLE;
          break;
      case "actionbar":
          action = Action.ACTIONBAR;
          break;
      default:
          throw new InternalExpressionException("'display_title' requires 'title', 'subtitle' or 'actionbar' as second argument");
  }
  if (times.length == 3)
      targets.forEach(p -> p.networkHandler.sendPacket(new TitleS2CPacket(Action.TIMES, null, times[0], times[1], times[2])));
  targets.forEach(p -> p.networkHandler.sendPacket(new TitleS2CPacket(action, content)));
  return true;
}

Current display_title for comparison:

expression.addLazyFunction("display_title", -1, (c, t, lv) -> {
    if (lv.size() < 2) throw new InternalExpressionException("'display_title' needs at least a target, type and message, and optionally times");
    Value pVal = lv.get(0).evalValue(c);
    if (!(pVal instanceof ListValue)) pVal = ListValue.of(pVal);
    MinecraftServer server = ((CarpetContext)c).s.getMinecraftServer();
    Stream<ServerPlayerEntity> targets = ((ListValue) pVal).getItems().stream().map(v ->
    {
        ServerPlayerEntity player = EntityValue.getPlayerByValue(server, v);
        if (player == null) throw new InternalExpressionException("'display_title' requires a valid online player or a list of players as first argument. "+v.getString()+" is not a player.");
        return player;
    });
    TitleS2CPacket.Action action;
    switch (lv.get(1).evalValue(c).getString().toLowerCase(Locale.ROOT))
    {
        case "title":
            action = Action.TITLE;
            break;
        case "subtitle":
            action = Action.SUBTITLE;
            break;
        case "actionbar":
            action = Action.ACTIONBAR;
            break;
        case "clear":
            action = Action.CLEAR;
            break;
        default:
            throw new InternalExpressionException("'display_title' requires 'title', 'subtitle', 'actionbar' or 'clear' as second argument");
    }
    if (action != Action.CLEAR && lv.size() < 3)
        throw new InternalExpressionException("Third argument of 'display_title' must be present except for 'clear' type");
    Text title = null;
    if (lv.size() > 2)
    {
        pVal = lv.get(2).evalValue(c);
        if (pVal instanceof FormattedTextValue)
            title = ((FormattedTextValue) pVal).getText();
        else
            title = new LiteralText(pVal.getString());
    }
    TitleS2CPacket timesPacket;
    if (lv.size() > 3)
    {
        int in = NumericValue.asNumber(lv.get(3).evalValue(c),"fade in for display_title" ).getInt();
        int stay = NumericValue.asNumber(lv.get(4).evalValue(c),"stay for display_title" ).getInt();
        int out = NumericValue.asNumber(lv.get(5).evalValue(c),"fade out for display_title" ).getInt();
        timesPacket = new TitleS2CPacket(Action.TIMES, null, in, stay, out);
    }
    else timesPacket = null;

    TitleS2CPacket packet = new TitleS2CPacket(action, title);
    AtomicInteger total = new AtomicInteger(0);
    targets.forEach(p -> {
        if (timesPacket != null) p.networkHandler.sendPacket(timesPacket);
        p.networkHandler.sendPacket(packet);
        total.getAndIncrement();
    });
    Value ret = NumericValue.of(total.get());
    return (cc, tt) -> ret;
});

Current performance (only tested this specific function) seems to be of about 7/11 runs per tick, which I think is quite good, given there are currently some streams being used in hot code that could be replaced with simpler loops.

*Only thing missing is making Text optional, for the clear function.