godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.1k stars 69 forks source link

Add `optional arguments` in `Array.sort_custom()` #7872

Open Arecher opened 11 months ago

Arecher commented 11 months ago

Describe the project you are working on

Game Project in Godot 3.5

Describe the problem or limitation you are having in your project

When needing to resort to a sort_custom() function for my Array sorting needs, I found that I was very frequently rewriting an extremely similar sort function, often with only a few key variables being changed out. Having 4+ functions that are essentially copy-pasted, performing the same logic on a different variable is always bad practice, and with sort_custom there is simply no way around it.

For example, I have an Array of fruit that I would like to sort, based on data available in a dictionary.

var fruitarray = ["apples", "oranges", "pears"]
var fruitcosts = {"apples": 4, "grapefruits": 12, "kiwis": 8, "oranges": 10, "pears": 7}

To sort them from cheapest to most expensive, we'd simply write a custom_sort function.

func sort_cost(a, b):
    if fruitcosts[a] < fruitcosts[b]:
        return true
    return false

fruitarray.sort_custom(self, "sort_cost")

Now if we add another dictionary, say fruitinventory, and we want to sort a different fruitarray based on the amount in our little shop's inventory, we'd have to add:

var newfruitarray = ["apples", "oranges", "pears"]
var fruitinventory= {"apples": 33, "grapefruits": 4, "kiwis": 16, "oranges": 5, "pears": 1}

func sort_inventorycount(a, b):
    if fruitinventory[a] < fruitinventory[b]:
        return true
    return false

newfruitarray.sort_custom(self, "sort_inventorycount")

Which duplicates the amount of functions and clutter, even though the function is nearly identical. And this is only a tiny function. For longer/more complex sorting, having to add multiple near identical functions can feel very bad and feels like bad practice.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Allow sort_custom to take optional arguments. Meaning it would become

Array.sort_custom(self, "sort_function", optionalarg1, ...)

or

Array.sort_custom(self, "sort_function", [optionalarg1, ...])

Which would call:

func sort_function(a, b, optionalarg1):
    if optionalarg1[a] < optionalarg1[b]:
        return true
    return false

This would allow users to handle sorting multiple Arrays with similar required logic, via a single function.

This optional argument logic is already used in various other places throughout Godot, such as signals. I'm unsure whether allowing additional arguments via an Array or vararg would be better, since both are used in the Engine.

I am aware that in Godot 4.x sort_custom() is changed to be a Callable function, but from my quick research this seems to not resolve this issue. It changes how the function is called, but still limits the function to two arguments, a & b. The issue might be ever so slightly mitigated for tiny sorting functions, since they can be written as Lambda functions. However, having one centralized sorting function with an optional argument would be preferred to me, over having multiple similar Lambda functions whenever sort_custom is called.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

See above. I lack the in-depth knowledge of the Backend of Godot to provide proper implementation context.

If this enhancement will not be used often, can it be worked around with a few lines of script?

It can be worked around by duplicating your sorting functions.

I suspect it would be used fairly often, for most slightly more complex or indirect sort_custom() cases.

Is there a reason why this should be core and not an add-on in the asset library?

Array.sort_custom() is core.

mieldepoche commented 11 months ago

I guess it would be nice.

In 3.x, what I would do is do a central sorting method and make submethods that call the main one (this avoid having to copy all the code over and over, but it's still tedious):

func sort_generic(a, b, other arguments) -> bool:
    return [complex condition that depends on a, b, other arguments]

func sort_specific_1(a, b) -> bool:
    return sort_generic(a, b, specific arguments)

# etc.

# and then:
array.sort_custom(self, "sort_specific_1")

In 4.x you can bind arguments, it's a satisfactory solution imo:

@tool
extends EditorScript

var fruitarray := ["apples", "oranges", "pears"]
var fruitcosts := {"apples": 4, "grapefruits": 12, "kiwis": 8, "oranges": 10, "pears": 7}
var fruitinventory := {"apples": 33, "grapefruits": 4, "kiwis": 16, "oranges": 5, "pears": 1}

func sort_fruits(a: String, b: String, map: Dictionary) -> bool:
    return map[a] < map[b]

func _run() -> void:
    fruitarray.sort()
    print("fruits: ", fruitarray)

    fruitarray.sort_custom( sort_fruits.bind(fruitcosts) )
    print("sorted by cost:")
    print(fruitarray)

    fruitarray.sort_custom( sort_fruits.bind(fruitinventory) )
    print("sorted by inventory amount:")
    print(fruitarray)
wareya commented 11 months ago

Can you do this already with Callable.bind()?

mieldepoche commented 11 months ago

Can you do this already with Callable.bind()?

yes (see my example above) but op is using 3.5

Arecher commented 11 months ago

With the workarounds/solutions mentioned above, I think adding optional arguments to sort_custom() is less required than I had initially thought. However, I still think it would be a logical thing to allow, and I suspect it would still be used even with the current other solutions. Especially in 3.x it could make sorting feel less like a workaround.

In 4.x binds seem a very powerful feature to combat the issue, but I do wonder how approachable the solution is to newer users. Meaning, I'd expect new/intermediate users to sooner look for ways to add optional arguments into their sort_custom() function, over looking into bind. You'd have to already be familiar with binds to see that they are the answer to your problem! It seems like a large mental leap to make for uses that aren't yet familiar. However, perhaps this could be mitigated by mentioning bind() in an example in the documentation for sort_custom() in 4..x

But as I haven't been able to use 4.x yet, it's harder for me to assess whether optional arguments for sort_custom would be desirable/required to the average user in that version, or whether binds are the more logical solution for them.

kleonc commented 11 months ago

If this enhancement will not be used often, can it be worked around with a few lines of script?

It can be worked around by duplicating your sorting functions.

I suspect it would be used fairly often, for most slightly more complex or indirect sort_custom() cases.

Another possible solution could be adding a utility class like:

class_name Sorter

static func with_data(data):
    return SorterWithData.new(data)

class SorterWithData:
    var data

    func _init(data) -> void:
        self.data = data

    func sort_type_a(a, b):
        return data[a] < data[b]

#   func sort_type_b(a, b):
#       ...

Usage (3.x):

var fruitarray = ["apples", "oranges", "pears"]
var fruitcosts = {"apples": 4, "grapefruits": 12, "kiwis": 8, "oranges": 10, "pears": 7}

var newfruitarray = ["apples", "oranges", "pears"]
var fruitinventory= {"apples": 33, "grapefruits": 4, "kiwis": 16, "oranges": 5, "pears": 1}

fruitarray.sort_custom(Sorter.with_data(fruitcosts), "sort_type_a")
newfruitarray.sort_custom(Sorter.with_data(fruitinventory), "sort_type_a")

Such utility class could be of course tweaked/improved (e.g. by providing available sorting types as string constants, or whatever else is needed/preferred).