Return control flow separately from branch instructions

[swazrgb]

This gives access to the value even when branching multiple times, such as selectNearest which has 3 branches and a return value

[swazrgb]

Perhaps the old syntax is preferable when the instruction only has two branches, though there are (or at least could be) some instructions that return a value for both branches. Please let me know what you think. Thanks!

[swazrgb]

I realized that if #44 were to be merged, we would likely also be able to support syntax like this instead, which might be the best of both worlds:

declare function selectNearest(unit_a: Value, unit_b: Value): Value & ("next" | "A" | "B");

function test(p1: Value, p2: Value) {
    const nearest = selectNearest(p1, p2);
    switch(nearest) {
        case "A":
            notify("A", nearest);
            break;
        case "B":
            notify("B", nearest);
            break;
        case "next":
            notify("Next");
            break;
    }
}

This does however have some typing quirks, e.g.:

function test(p1: Value, p2: Value, nearest: Value) {
    nearest = selectNearest(p1, p2);
    switch(nearest) {
        case "A":
            notify("A", nearest);
            break;
        case "B":
            notify("B", nearest);
            break;
        case "next":
            notify("Next");
            break;
    }
}

Doesn't get typechecked properly. So maybe not.

[swazrgb]

@ribrdb Please disregard my previous comment. I think this implementation is the best it's going to get:

export function foo(v:Value) {
    let [branch, nearest] = selectNearest(v, signal);
    switch(branch) {
        case "A":
            notify("A is closest", nearest);
            break;
        case "B":
            notify("B is closest", nearest);
            break;
        default:
            notify("Default branch");
            break;
    }
}

[swazrgb]

We should probably also add another override that only returns the nearest entity without the branching value.

Adding an overload would require naming the two functions differently, since they have the same arguments but different return types. Easy enough to do via an override, but do you have a suggestion for the naming? I suppose selectNearest and selectNearestBranch could work, but personally I don't think this is all that more clear than just having a single function that returns an array, which other functions do as well.

Perhaps it would help if the control flow argument came last? That way the user could write the following when they are not interested in the control flow argument:

let [nearest] = selectNearest(a, b);

Thanks for taking a look!

[ribrdb]

We should probably also add another override that only returns the nearest entity without the branching value.

Adding an overload would require naming the two functions differently, since they have the same arguments but different return types. Easy enough to do via an override, but do you have a suggestion for the naming? I suppose selectNearest and selectNearestBranch could work, but personally I don't think this is all that more clear than just having a single function that returns an array, which other functions do as well.

How about closest(a,b) to just return the nearest value, and selectNearest to return both? That naming matches the dom api, so might be more familiar to js users. I guess we could also just have nearest and selectNearest.

Perhaps it would help if the control flow argument came last? That way the user could write the following when they are not interested in the control flow argument:

let [nearest] = selectNearest(a, b);

Thanks for taking a look!

What's weird about this function is that it's basically returning the data in two different formats. That kind of makes sense in the visual programming environment, although it's also pretty different from every other Desynced instruction. I can't think of any commonly used function in any standard programming language that returns a single value in multiple formats like that, and it seems to me that the typical use case is that you want one or the other. So needing to add brackets to get out a single value still feels awkward.

That said, I don't think figuring out the other variant needs to block this pr.

[swazrgb]

How about closest(a,b) to just return the nearest value, and selectNearest to return both? That naming matches the dom api, so might be more familiar to js users. I guess we could also just have nearest and selectNearest.

These names are fine in principle, but personally I'd prefer for the desynced instruction names to somewhat resemble the in-game instruction names. I assume most users come from the visual behavior editor, and thus will be looking for those names as functions. It would also be awkward if the game ever did introduce a closest instruction for something else (I'm not sure what, but it could happen)

What about:

declare function selectNearest(unit_a: Value, unit_b: Value): ["next" | "A" | "B", Value];
declare function selectNearestEntity(unit_a: Value, unit_b: Value): Value;

interface BaseValue {
  isNearer(other: Value): boolean;
}

This would let selectNearest match the game behavior, as it does in the PR currently. It would also introduce two ergonomic functions: selectNearestEntity, which simply returns the closest of the two and the Valuemethod isNearer which returns a boolean where true maps to the A branch, and false to B and None.

That said, I don't think figuring out the other variant needs to block this pr.

If you agree with the above, I would indeed like to merge this PR now and introduce selectNearestEntity and isNearer in a future PR.

What's weird about this function is that it's basically returning the data in two different formats. That kind of makes sense in the visual programming environment, although it's also pretty different from every other Desynced instruction

I understand the feeling, and while it's true this is the only current instruction the game has that behaves this way, it would not surprise me at all if more appeared in the future. The behavior engine is fine with any instruction having an arbitrary number of variables and outputs.

But yes, it's unusual. It is also the target of the compiler, and especially considering that I'd love to add support for modded instructions at some point, it would be good to be able to support as much of the behavior engine as possible.

I can't think of any commonly used function in any standard programming language that returns a single value in multiple formats like that, and it seems to me that the typical use case is that you want one or the other

Me neither, but I don't see desynced-tools as a general purpose programming language. I see it as a DSL to generate Desynced behaviors, so I'm willing to tolerate a bit of weirdness.

[ribrdb]

What about:

declare function selectNearest(unit_a: Value, unit_b: Value): ["next" | "A" | "B", Value];
declare function selectNearestEntity(unit_a: Value, unit_b: Value): Value;

interface BaseValue {
  isNearer(other: Value): boolean;
}

That looks good. Is there a reason you prefer isNearer over the current nearerThan?

If you agree with the above, I would indeed like to merge this PR now and introduce selectNearestEntity and isNearer in a future PR. 👍

Me neither, but I don't see desynced-tools as a general purpose programming language. I see it as a DSL to generate Desynced behaviors, so I'm willing to tolerate a bit of weirdness.

I guess. But I feel like since we're already running a compiler that does optimizations, we should let the compiler worry about the exactly which instructions get output and the "dsl" just expresses the ideas as naturally as possible. But mods is a good point, that's not really feasible to do when you consider mod instructions.

[swazrgb]

Is there a reason you prefer isNearer over the current nearerThan?

Not really, either is fine with me. I typically call functions that return a boolean is, and perhaps it could be confusing if the same function exists both in the global namespace and as a method on a Value, but I don't mind.

I guess. But I feel like since we're already running a compiler that does optimizations, we should let the compiler worry about the exactly which instructions get output and the "dsl" just expresses the ideas as naturally as possible.

Fair point. Onboarding is somewhat of a concern, but some good examples can solve that.

[swazrgb]

Sorry, I completely blanked on the fact that nearerThan already exists in the global scope. It had been a while since I worked on this PR :D

That method is fine, but I do feel it would perhaps be more natural to write if(a.isNearer(b)), but either is fine.