godotengine / godot-visual-script

VisualScript as a Godot Engine c++ module
MIT License
119 stars 22 forks source link

VisualScript node for array access of an element by index #15

Open InkTide opened 4 years ago

InkTide commented 4 years ago

Describe the project you are working on: Any project using VisualScript. Describe the problem or limitation you are having in your project: The existing nodes for interacting with arrays have no way to access a single element of an array using the element's index within that array. The node-based workaround creates unnecessary iterators, harming performance. Describe how this feature / enhancement will help you overcome this problem or limitation: A node that functions the same way as a "var = array[n]" statement in GDScript would entirely eliminate the problem. Show a mock up screenshots/video or a flow diagram explaining how your proposal will work: Arrays in VisualScript should have a node that takes two inputs, an array and an integer, and outputs the data stored at the index corresponding to that integer within the array. If the integer is greater than the size of the array, NULL could be returned. Describe implementation detail for your proposal (in code), if possible: See above, it's really just a node for "var = array[n]" with a bit of bounds checking. VisualScript already has access to all of GDScript's array functions, but none of those methods approximate "array[n]". In order to fully match the usefulness of "array[n]", a second node could be added that would have three inputs: the array, the index integer, and a variable. This would set the element at the index integer in the array to the input variable. Then it might return the edited array. In essence, a node-based way to implement "array[n] = var," though admittedly the node-based workaround for this is fairly simple ("array.insert(n, var)" followed by "array.remove(n+1)").

I'm not sure how the Godot engine implements VisualScript nodes under the hood, so here's some pseudocode:

Array element 'n' getter:

element_get(Array a, int index) { if (index >= a.size() || index < 0) { return NULL; } else { return a[index]; } }

Array element 'n' setter:

element_set(Array a, int index, var data) { if (index >= a.size() || index < 0) { return NULL; } else { a[index] = data; return a; } } If this enhancement will not be used often, can it be worked around with a few lines of script?: It is an extremely common use for arrays, and the current node-based workaround is a dedicated function that copies the array, loops through it by popping the front and decrementing the index, then returns the value at index 0 in the copied array once the index argument equals zero - a mountain of nodes and wasted computational power. Is there a reason why this should be core and not an add-on in the asset library?: VisualScript's interaction with arrays without such a node is severely limited, as accessing the array becomes needlessly obtuse, which is something VisualScript should try to avoid as part of its design philosophy.

fire commented 4 years ago

image

Which nodes?

InkTide commented 4 years ago

image

Which nodes?

Neither of those are, as far as I'm aware, capable of outputting a single element of an array. Array.find() outputs the index, and Compose outputs the entire array.

InkTide commented 4 years ago

Doesn't that just make an array with one element? That just seems like the VisualScript version of "var out = [arg]".

fire commented 4 years ago

image

I need to investigate this node.

fire commented 4 years ago

https://github.com/godotengine/godot/blob/784595fda1ab1c933888da688a6debcf785e3fec/modules/visual_script/visual_script_nodes.cpp#L1716

https://github.com/godotengine/godot/blob/master/core/array.cpp#L209-L212

[Edited] Seems to do what you want.

It's a bit difficult to use, dragging out should tell you what type it is.

InkTide commented 4 years ago

https://github.com/godotengine/godot/blob/784595fda1ab1c933888da688a6debcf785e3fec/modules/visual_script/visual_script_nodes.cpp#L1716

https://github.com/godotengine/godot/blob/master/core/array.cpp#L209-L212

[Edited] Seems to do what you want.

It's a bit difficult to use, dragging out should tell you what type it is.

Ah, so that's what that node does. A basic element accessor for anything with multiple members, including arrays, if I'm understanding it correctly. I thought it was for getting the index, like Array.find(). Perhaps it and Set Index should be renamed "Get Element" and "Set Element", or maybe even "Get Element at Index" and "Set Element at Index."

Does that do bounds checking at all? What happens when you use non-integer variables for the index, or a variable without members like an integer for the "base"?

InkTide commented 4 years ago

On second thought, maybe Get/Set Member would fit better, not sure which jargon GDScript prefers.

swarnimarun commented 4 years ago

@InkTide Actually that's a base implementation, additional nodes would have to be built on top.

I did plan to add more Array operation nodes but it's just a lot of tedious work, albeit not very hard, with all the things that we already have in place.