Godot: Unexpected returns from array.slice() if end > array.size()-1

Created on 11 Dec 2019  路  3Comments  路  Source: godotengine/godot

Godot version:
Godot_v3.2-beta3_win64

OS/device including version:
Windows 10

Issue description:
Running this code

func _ready():  
    var array = [0,1,2]  
    for i in range(-2, 3):  
        var slice = array.slice(0, array.size() + i)  
        print(slice)

prints
[0, 1]
[0, 1, 2]
[0, 1, 2, 2]
[0, 1, 2, 2]
[0, 1, 2, 2]

It seems to return with a copy of the final element at the end of the array when you make 'end' higher than the array's max index. This only happens if 'step' is 1.
If you change array to [[0,0],[1,1],[2,2]] you get
[[0, 0], [1, 1]]
[[0, 0], [1, 1], [2, 2]]
[[0, 0], [1, 1], [2, 2], [...]]
[[0, 0], [1, 1], [2, 2], [...]]
[[0, 0], [1, 1], [2, 2], [...]]
getting the [...] returns [2, 2] (slice[-1]) (slice.size() returns 4)

bug confirmed core

Most helpful comment

I've proposed a re-implementation of Array.slice() to address these and a few other issues I've found with the indexing.

In doing so, I've also modified the method so the end (aka stop) parameter is exclusive. This could be considered a _breaking change_, however:

  • This is consistent with other scripting languages that implement a .slice(begin, end) method (e.g., Python, JavaScript)
  • This is consistent with GDScript's range(initial, final, increment) implementation, where the final element generated is final-1.

It seems logical (although up for debate) that getting a subset of a list using range() and slice() using similar inputs would yield the same results:

var a = [0,1,2,3,4,5]
#Using Range
for i in range(0,3):
    print(a[i])

#Using Slice
for i in a.slice(0,3):
    print(a[i]) #Expect similar output?

Array.slice() currently returns an extra element , but with this pull request the results of the two approaches would be brought to parity.

Here are some additional details on updated behavior using 14 scenarios:

Cases where behavior will change:
| # | Code | Current | Proposed |
| --- | ---- | ------- | --- |
| 1 | [0, 1, 2, 3, 4, 5].slice(1, 4) | [1, 2, 3, 4] | [1, 2, 3] |
| 2 | [0, 1, 2, 3, 4, 5].slice(0, 6) | [0, 1, 2, 3, 4, 5, 5] | [0, 1, 2, 3, 4, 5] |
| 3 | [0, 1, 2, 3, 4, 5].slice(0, 6, 2) | [0, 2, 4, 5] | [0, 2, 4] |
| 4 | [0, 1, 2, 3, 4, 5].slice(6, 0, -1) | [5, 4, 3, 2, 1, 0] | [5, 4, 3, 2, 1] |
| 5 | [0, 1, 2, 3, 4, 5].slice(2, 2) | [2] | [] |
| 6 | [0, 1].slice(0, 1, 2) | [] | [0] |
| 7 | [0, 1].slice(0, 2, 2) | [0, 1] | [0] |
| 8 | [0, 1, 2, 3, 4, 5].slice(-4, -1) | [2, 3, 4, 5] | [2, 3, 4] |
| 9 | [0, 1, 2, 3, 4, 5].slice(-1, -4, -1) | [5, 4, 3, 2] | [5, 4, 3] |

Cases 2, 3, 6, 7 were previously identified in this issue discussion.

Cases where behavior will not change:
| # | Code | Current | Proposed |
| --- | ---- | ------- | --- |
| 10 | [0, 1, 2, 3, 4, 5].slice(6, 0) | [] | [] |
| 11 | [0, 1, 2, 3, 4, 5].slice(6, 0, -2) | [5, 3, 1] | [5, 3, 1] |
| 12 | [0, 1, 2, 3, 4, 5].slice(0, 6, 0) | ERROR -> [] | ERROR -> [] |
| 13 | [0, 1, 2, 3, 4, 5].slice(-4, -1, -1) | [] | [] |
| 14 | [0, 1, 2, 3, 4, 5].slice(-1, -4) | [] | [] |

Perhaps of note, I've used CPython as the reference implementation ("lifted directly"?). Running these inputs in Python using the list[start:stop:step] notation will give identical results to the proposed state, with the exception of Case 12. If given a step of size 0, Python will error and stop processing, where the proposed implementation will error but continue processing with an empty Array.

All 3 comments

This bug still exists in the 3.2.1 rc2 release.
The issue is obvious in the source code. However, the implementation of this method seems a bit too complicated and error prone compared to other source code in Array.cpp.

There's also this thing that is similar to this issue:

print([0, 1, 2, 3, 4, 5].slice(0, 6)) # [0, 1, 2, 3, 4, 5, 5]
print([0, 1, 2, 3, 4, 5].slice(0, 6, 2)) # [0, 2, 4, 5]

The first line shows the bug described here. The second one takes every second element but if the end index is off by one, it also grabs that 5 in the end, which it probably shouldn't.

Additional observations:

  • [0, 1].slice(0, 1, 2) case returns [] while 0 is technically the first of every second element that the array can possibly have (at least it was my expectation).
  • [0, 1].slice(0, 2, 2) returns [0, 1].

I've proposed a re-implementation of Array.slice() to address these and a few other issues I've found with the indexing.

In doing so, I've also modified the method so the end (aka stop) parameter is exclusive. This could be considered a _breaking change_, however:

  • This is consistent with other scripting languages that implement a .slice(begin, end) method (e.g., Python, JavaScript)
  • This is consistent with GDScript's range(initial, final, increment) implementation, where the final element generated is final-1.

It seems logical (although up for debate) that getting a subset of a list using range() and slice() using similar inputs would yield the same results:

var a = [0,1,2,3,4,5]
#Using Range
for i in range(0,3):
    print(a[i])

#Using Slice
for i in a.slice(0,3):
    print(a[i]) #Expect similar output?

Array.slice() currently returns an extra element , but with this pull request the results of the two approaches would be brought to parity.

Here are some additional details on updated behavior using 14 scenarios:

Cases where behavior will change:
| # | Code | Current | Proposed |
| --- | ---- | ------- | --- |
| 1 | [0, 1, 2, 3, 4, 5].slice(1, 4) | [1, 2, 3, 4] | [1, 2, 3] |
| 2 | [0, 1, 2, 3, 4, 5].slice(0, 6) | [0, 1, 2, 3, 4, 5, 5] | [0, 1, 2, 3, 4, 5] |
| 3 | [0, 1, 2, 3, 4, 5].slice(0, 6, 2) | [0, 2, 4, 5] | [0, 2, 4] |
| 4 | [0, 1, 2, 3, 4, 5].slice(6, 0, -1) | [5, 4, 3, 2, 1, 0] | [5, 4, 3, 2, 1] |
| 5 | [0, 1, 2, 3, 4, 5].slice(2, 2) | [2] | [] |
| 6 | [0, 1].slice(0, 1, 2) | [] | [0] |
| 7 | [0, 1].slice(0, 2, 2) | [0, 1] | [0] |
| 8 | [0, 1, 2, 3, 4, 5].slice(-4, -1) | [2, 3, 4, 5] | [2, 3, 4] |
| 9 | [0, 1, 2, 3, 4, 5].slice(-1, -4, -1) | [5, 4, 3, 2] | [5, 4, 3] |

Cases 2, 3, 6, 7 were previously identified in this issue discussion.

Cases where behavior will not change:
| # | Code | Current | Proposed |
| --- | ---- | ------- | --- |
| 10 | [0, 1, 2, 3, 4, 5].slice(6, 0) | [] | [] |
| 11 | [0, 1, 2, 3, 4, 5].slice(6, 0, -2) | [5, 3, 1] | [5, 3, 1] |
| 12 | [0, 1, 2, 3, 4, 5].slice(0, 6, 0) | ERROR -> [] | ERROR -> [] |
| 13 | [0, 1, 2, 3, 4, 5].slice(-4, -1, -1) | [] | [] |
| 14 | [0, 1, 2, 3, 4, 5].slice(-1, -4) | [] | [] |

Perhaps of note, I've used CPython as the reference implementation ("lifted directly"?). Running these inputs in Python using the list[start:stop:step] notation will give identical results to the proposed state, with the exception of Case 12. If given a step of size 0, Python will error and stop processing, where the proposed implementation will error but continue processing with an empty Array.

Was this page helpful?
0 / 5 - 0 ratings