Fix range error for `Array.slice` #79103

AThousandShips · 2023-07-06T13:16:12Z

Start of the range is included so the upper end of the range should be clamped or it will try to access the element beyond the end, the same error shouldn't occur in 3.x as it does the correct clamping there

Added a unit test to catch the crash or behavior, could replace the previous one just to simplify as well

I also have no idea how I didn't notice this one when I did my previous PR for slice

Fixes: Slice array with negative step and out of bound index crash the engine #79093
Fixes: Impossible to get the last element of an array using negative indices in ARRAY.slice() #61530

kleonc

I don't think this is a proper fix. In #56668 slice was changed to work similarily to e.g. Python. With this PR e.g.:

[0].slice(1) would return [0] instead of an empty array,
[].slice(anything, anything2) would crash (as it would result in begin = -1, end = 0 because of how CLAMP works).

AThousandShips · 2023-07-06T14:10:10Z

Good points will investigate further

Too hasty review, bugs were found.

AThousandShips · 2023-07-06T14:31:17Z

Doing it conditionally only when p_step is negative seems to work correctly, will test further

AThousandShips · 2023-07-06T14:38:52Z

There, I believe it works correctly now, tested with the examples you gave and added some new unit tests, should be correct now I think

kleonc · 2023-07-07T10:12:22Z

Here's a comparison of the current state of this PR with the Python's slicing notation (iterable[begin:end:step]):

Generating `slice_gdscript.txt`

@tool
extends EditorScript

func _run() -> void:
	var s := 3
	var a := range(s)

	var f := FileAccess.open("res://slice_gdscript.txt", FileAccess.WRITE)
	for begin in range(-2 * s, 2 * s + 1):
		for end in range(-2 * s, 2 * s + 1):
			for step in range(-2 * s, 2 * s + 1):
				if step == 0:
					continue
				var slice := a.slice(begin, end, step)
				var line := "%s.slice(%2s, %2s, %2s) = %s" % [a, begin, end, step, slice]
				f.store_line(line)

Generating `slice_python.txt`

a = list(range(3))
s = len(a)

text = ""

for begin in range(-2 * s, 2 * s + 1):
	for end in range(-2 * s, 2 * s + 1):
		for step in range(-2 * s, 2 * s + 1):
			if step == 0:
				continue
			slice = a[begin:end:step]
			text += f"{a}.slice({begin:2}, {end:2}, {step:2}) = {slice}\n"

from pathlib import Path
p = Path(__file__).with_name('slice_python.txt')
p.write_text(text)

git diff -U0 slice_gdscript.txt slice_python.txt

diff --git a/slice_gdscript.txt b/slice_python.txt
index a33c575..778628a 100644
--- a/slice_gdscript.txt
+++ b/slice_python.txt
@@ -469,6 +469,6 @@
-[0, 1, 2].slice(-3, -6, -6) = []
-[0, 1, 2].slice(-3, -6, -5) = []
-[0, 1, 2].slice(-3, -6, -4) = []
-[0, 1, 2].slice(-3, -6, -3) = []
-[0, 1, 2].slice(-3, -6, -2) = []
-[0, 1, 2].slice(-3, -6, -1) = []
+[0, 1, 2].slice(-3, -6, -6) = [0]
+[0, 1, 2].slice(-3, -6, -5) = [0]
+[0, 1, 2].slice(-3, -6, -4) = [0]
+[0, 1, 2].slice(-3, -6, -3) = [0]
+[0, 1, 2].slice(-3, -6, -2) = [0]
+[0, 1, 2].slice(-3, -6, -1) = [0]
@@ -481,6 +481,6 @@
-[0, 1, 2].slice(-3, -5, -6) = []
-[0, 1, 2].slice(-3, -5, -5) = []
-[0, 1, 2].slice(-3, -5, -4) = []
-[0, 1, 2].slice(-3, -5, -3) = []
-[0, 1, 2].slice(-3, -5, -2) = []
-[0, 1, 2].slice(-3, -5, -1) = []
+[0, 1, 2].slice(-3, -5, -6) = [0]
+[0, 1, 2].slice(-3, -5, -5) = [0]
+[0, 1, 2].slice(-3, -5, -4) = [0]
+[0, 1, 2].slice(-3, -5, -3) = [0]
+[0, 1, 2].slice(-3, -5, -2) = [0]
+[0, 1, 2].slice(-3, -5, -1) = [0]
@@ -493,6 +493,6 @@
-[0, 1, 2].slice(-3, -4, -6) = []
-[0, 1, 2].slice(-3, -4, -5) = []
-[0, 1, 2].slice(-3, -4, -4) = []
-[0, 1, 2].slice(-3, -4, -3) = []
-[0, 1, 2].slice(-3, -4, -2) = []
-[0, 1, 2].slice(-3, -4, -1) = []
+[0, 1, 2].slice(-3, -4, -6) = [0]
+[0, 1, 2].slice(-3, -4, -5) = [0]
+[0, 1, 2].slice(-3, -4, -4) = [0]
+[0, 1, 2].slice(-3, -4, -3) = [0]
+[0, 1, 2].slice(-3, -4, -2) = [0]
+[0, 1, 2].slice(-3, -4, -1) = [0]
@@ -630 +630 @@
-[0, 1, 2].slice(-2, -6, -1) = [1]
+[0, 1, 2].slice(-2, -6, -1) = [1, 0]
@@ -642 +642 @@
-[0, 1, 2].slice(-2, -5, -1) = [1]
+[0, 1, 2].slice(-2, -5, -1) = [1, 0]
@@ -654 +654 @@
-[0, 1, 2].slice(-2, -4, -1) = [1]
+[0, 1, 2].slice(-2, -4, -1) = [1, 0]
@@ -785,2 +785,2 @@
-[0, 1, 2].slice(-1, -6, -2) = [2]
-[0, 1, 2].slice(-1, -6, -1) = [2, 1]
+[0, 1, 2].slice(-1, -6, -2) = [2, 0]
+[0, 1, 2].slice(-1, -6, -1) = [2, 1, 0]
@@ -797,2 +797,2 @@
-[0, 1, 2].slice(-1, -5, -2) = [2]
-[0, 1, 2].slice(-1, -5, -1) = [2, 1]
+[0, 1, 2].slice(-1, -5, -2) = [2, 0]
+[0, 1, 2].slice(-1, -5, -1) = [2, 1, 0]
@@ -809,2 +809,2 @@
-[0, 1, 2].slice(-1, -4, -2) = [2]
-[0, 1, 2].slice(-1, -4, -1) = [2, 1]
+[0, 1, 2].slice(-1, -4, -2) = [2, 0]
+[0, 1, 2].slice(-1, -4, -1) = [2, 1, 0]
@@ -937,6 +937,6 @@
-[0, 1, 2].slice( 0, -6, -6) = []
-[0, 1, 2].slice( 0, -6, -5) = []
-[0, 1, 2].slice( 0, -6, -4) = []
-[0, 1, 2].slice( 0, -6, -3) = []
-[0, 1, 2].slice( 0, -6, -2) = []
-[0, 1, 2].slice( 0, -6, -1) = []
+[0, 1, 2].slice( 0, -6, -6) = [0]
+[0, 1, 2].slice( 0, -6, -5) = [0]
+[0, 1, 2].slice( 0, -6, -4) = [0]
+[0, 1, 2].slice( 0, -6, -3) = [0]
+[0, 1, 2].slice( 0, -6, -2) = [0]
+[0, 1, 2].slice( 0, -6, -1) = [0]
@@ -949,6 +949,6 @@
-[0, 1, 2].slice( 0, -5, -6) = []
-[0, 1, 2].slice( 0, -5, -5) = []
-[0, 1, 2].slice( 0, -5, -4) = []
-[0, 1, 2].slice( 0, -5, -3) = []
-[0, 1, 2].slice( 0, -5, -2) = []
-[0, 1, 2].slice( 0, -5, -1) = []
+[0, 1, 2].slice( 0, -5, -6) = [0]
+[0, 1, 2].slice( 0, -5, -5) = [0]
+[0, 1, 2].slice( 0, -5, -4) = [0]
+[0, 1, 2].slice( 0, -5, -3) = [0]
+[0, 1, 2].slice( 0, -5, -2) = [0]
+[0, 1, 2].slice( 0, -5, -1) = [0]
@@ -961,6 +961,6 @@
-[0, 1, 2].slice( 0, -4, -6) = []
-[0, 1, 2].slice( 0, -4, -5) = []
-[0, 1, 2].slice( 0, -4, -4) = []
-[0, 1, 2].slice( 0, -4, -3) = []
-[0, 1, 2].slice( 0, -4, -2) = []
-[0, 1, 2].slice( 0, -4, -1) = []
+[0, 1, 2].slice( 0, -4, -6) = [0]
+[0, 1, 2].slice( 0, -4, -5) = [0]
+[0, 1, 2].slice( 0, -4, -4) = [0]
+[0, 1, 2].slice( 0, -4, -3) = [0]
+[0, 1, 2].slice( 0, -4, -2) = [0]
+[0, 1, 2].slice( 0, -4, -1) = [0]
@@ -1098 +1098 @@
-[0, 1, 2].slice( 1, -6, -1) = [1]
+[0, 1, 2].slice( 1, -6, -1) = [1, 0]
@@ -1110 +1110 @@
-[0, 1, 2].slice( 1, -5, -1) = [1]
+[0, 1, 2].slice( 1, -5, -1) = [1, 0]
@@ -1122 +1122 @@
-[0, 1, 2].slice( 1, -4, -1) = [1]
+[0, 1, 2].slice( 1, -4, -1) = [1, 0]
@@ -1253,2 +1253,2 @@
-[0, 1, 2].slice( 2, -6, -2) = [2]
-[0, 1, 2].slice( 2, -6, -1) = [2, 1]
+[0, 1, 2].slice( 2, -6, -2) = [2, 0]
+[0, 1, 2].slice( 2, -6, -1) = [2, 1, 0]
@@ -1265,2 +1265,2 @@
-[0, 1, 2].slice( 2, -5, -2) = [2]
-[0, 1, 2].slice( 2, -5, -1) = [2, 1]
+[0, 1, 2].slice( 2, -5, -2) = [2, 0]
+[0, 1, 2].slice( 2, -5, -1) = [2, 1, 0]
@@ -1277,2 +1277,2 @@
-[0, 1, 2].slice( 2, -4, -2) = [2]
-[0, 1, 2].slice( 2, -4, -1) = [2, 1]
+[0, 1, 2].slice( 2, -4, -2) = [2, 0]
+[0, 1, 2].slice( 2, -4, -1) = [2, 1, 0]
@@ -1409,2 +1409,2 @@
-[0, 1, 2].slice( 3, -6, -2) = [2]
-[0, 1, 2].slice( 3, -6, -1) = [2, 1]
+[0, 1, 2].slice( 3, -6, -2) = [2, 0]
+[0, 1, 2].slice( 3, -6, -1) = [2, 1, 0]
@@ -1421,2 +1421,2 @@
-[0, 1, 2].slice( 3, -5, -2) = [2]
-[0, 1, 2].slice( 3, -5, -1) = [2, 1]
+[0, 1, 2].slice( 3, -5, -2) = [2, 0]
+[0, 1, 2].slice( 3, -5, -1) = [2, 1, 0]
@@ -1433,2 +1433,2 @@
-[0, 1, 2].slice( 3, -4, -2) = [2]
-[0, 1, 2].slice( 3, -4, -1) = [2, 1]
+[0, 1, 2].slice( 3, -4, -2) = [2, 0]
+[0, 1, 2].slice( 3, -4, -1) = [2, 1, 0]
@@ -1565,2 +1565,2 @@
-[0, 1, 2].slice( 4, -6, -2) = [2]
-[0, 1, 2].slice( 4, -6, -1) = [2, 1]
+[0, 1, 2].slice( 4, -6, -2) = [2, 0]
+[0, 1, 2].slice( 4, -6, -1) = [2, 1, 0]
@@ -1577,2 +1577,2 @@
-[0, 1, 2].slice( 4, -5, -2) = [2]
-[0, 1, 2].slice( 4, -5, -1) = [2, 1]
+[0, 1, 2].slice( 4, -5, -2) = [2, 0]
+[0, 1, 2].slice( 4, -5, -1) = [2, 1, 0]
@@ -1589,2 +1589,2 @@
-[0, 1, 2].slice( 4, -4, -2) = [2]
-[0, 1, 2].slice( 4, -4, -1) = [2, 1]
+[0, 1, 2].slice( 4, -4, -2) = [2, 0]
+[0, 1, 2].slice( 4, -4, -1) = [2, 1, 0]
@@ -1721,2 +1721,2 @@
-[0, 1, 2].slice( 5, -6, -2) = [2]
-[0, 1, 2].slice( 5, -6, -1) = [2, 1]
+[0, 1, 2].slice( 5, -6, -2) = [2, 0]
+[0, 1, 2].slice( 5, -6, -1) = [2, 1, 0]
@@ -1733,2 +1733,2 @@
-[0, 1, 2].slice( 5, -5, -2) = [2]
-[0, 1, 2].slice( 5, -5, -1) = [2, 1]
+[0, 1, 2].slice( 5, -5, -2) = [2, 0]
+[0, 1, 2].slice( 5, -5, -1) = [2, 1, 0]
@@ -1745,2 +1745,2 @@
-[0, 1, 2].slice( 5, -4, -2) = [2]
-[0, 1, 2].slice( 5, -4, -1) = [2, 1]
+[0, 1, 2].slice( 5, -4, -2) = [2, 0]
+[0, 1, 2].slice( 5, -4, -1) = [2, 1, 0]
@@ -1877,2 +1877,2 @@
-[0, 1, 2].slice( 6, -6, -2) = [2]
-[0, 1, 2].slice( 6, -6, -1) = [2, 1]
+[0, 1, 2].slice( 6, -6, -2) = [2, 0]
+[0, 1, 2].slice( 6, -6, -1) = [2, 1, 0]
@@ -1889,2 +1889,2 @@
-[0, 1, 2].slice( 6, -5, -2) = [2]
-[0, 1, 2].slice( 6, -5, -1) = [2, 1]
+[0, 1, 2].slice( 6, -5, -2) = [2, 0]
+[0, 1, 2].slice( 6, -5, -1) = [2, 1, 0]
@@ -1901,2 +1901,2 @@
-[0, 1, 2].slice( 6, -4, -2) = [2]
-[0, 1, 2].slice( 6, -4, -1) = [2, 1]
+[0, 1, 2].slice( 6, -4, -2) = [2, 0]
+[0, 1, 2].slice( 6, -4, -1) = [2, 1, 0]

So while crashes are gone the above diff shows the problem with the current implementation (existing before this PR): for a negative step it's impossible to make the resulting slice include the first element of the array. Probably a good idea to fix it in this PR.

Also some cases result in unneeded errors, e.g. [].slice(0, 0, -1) results in "Slice is negative, but bounds is increasing.".

AThousandShips · 2023-07-07T10:14:54Z

Will see what I can do, that's tracked in #61530

kleonc · 2023-07-07T10:19:01Z

Will see what I can do, that's tracked in #61530

Oh, I even already commented in there. 😆 But haven't noticed the problem with negative step back then though...

AThousandShips · 2023-07-07T10:52:03Z

Got a basic idea going, will test it when I'm able to be by my computer properly

AThousandShips · 2023-07-07T11:16:04Z

I believe this should do it, will add to the documentation as well later

Currently unable to be by my computer for more than a few minutes at a time so can't test it myself properly rn

kleonc · 2023-07-07T11:32:23Z

I believe this should do it, will add to the documentation as well later

Currently unable to be by my computer for more than a few minutes at a time so can't test it myself properly rn

With the recent changes the same diff as in #79103 (comment) is:

git diff -U0 slice_gdscript.txt slice_python.txt

diff --git a/slice_gdscript.txt b/slice_python.txt
index 0e21a82..778628a 100644
--- a/slice_gdscript.txt
+++ b/slice_python.txt
@@ -1,6 +1,6 @@
-[0, 1, 2].slice(-6, -6, -6) = [0]
-[0, 1, 2].slice(-6, -6, -5) = [0]
-[0, 1, 2].slice(-6, -6, -4) = [0]
-[0, 1, 2].slice(-6, -6, -3) = [0]
-[0, 1, 2].slice(-6, -6, -2) = [0]
-[0, 1, 2].slice(-6, -6, -1) = [0]
+[0, 1, 2].slice(-6, -6, -6) = []
+[0, 1, 2].slice(-6, -6, -5) = []
+[0, 1, 2].slice(-6, -6, -4) = []
+[0, 1, 2].slice(-6, -6, -3) = []
+[0, 1, 2].slice(-6, -6, -2) = []
+[0, 1, 2].slice(-6, -6, -1) = []
@@ -13,6 +13,6 @@
-[0, 1, 2].slice(-6, -5, -6) = [0]
-[0, 1, 2].slice(-6, -5, -5) = [0]
-[0, 1, 2].slice(-6, -5, -4) = [0]
-[0, 1, 2].slice(-6, -5, -3) = [0]
-[0, 1, 2].slice(-6, -5, -2) = [0]
-[0, 1, 2].slice(-6, -5, -1) = [0]
+[0, 1, 2].slice(-6, -5, -6) = []
+[0, 1, 2].slice(-6, -5, -5) = []
+[0, 1, 2].slice(-6, -5, -4) = []
+[0, 1, 2].slice(-6, -5, -3) = []
+[0, 1, 2].slice(-6, -5, -2) = []
+[0, 1, 2].slice(-6, -5, -1) = []
@@ -25,6 +25,6 @@
-[0, 1, 2].slice(-6, -4, -6) = [0]
-[0, 1, 2].slice(-6, -4, -5) = [0]
-[0, 1, 2].slice(-6, -4, -4) = [0]
-[0, 1, 2].slice(-6, -4, -3) = [0]
-[0, 1, 2].slice(-6, -4, -2) = [0]
-[0, 1, 2].slice(-6, -4, -1) = [0]
+[0, 1, 2].slice(-6, -4, -6) = []
+[0, 1, 2].slice(-6, -4, -5) = []
+[0, 1, 2].slice(-6, -4, -4) = []
+[0, 1, 2].slice(-6, -4, -3) = []
+[0, 1, 2].slice(-6, -4, -2) = []
+[0, 1, 2].slice(-6, -4, -1) = []
@@ -157,6 +157,6 @@
-[0, 1, 2].slice(-5, -6, -6) = [0]
-[0, 1, 2].slice(-5, -6, -5) = [0]
-[0, 1, 2].slice(-5, -6, -4) = [0]
-[0, 1, 2].slice(-5, -6, -3) = [0]
-[0, 1, 2].slice(-5, -6, -2) = [0]
-[0, 1, 2].slice(-5, -6, -1) = [0]
+[0, 1, 2].slice(-5, -6, -6) = []
+[0, 1, 2].slice(-5, -6, -5) = []
+[0, 1, 2].slice(-5, -6, -4) = []
+[0, 1, 2].slice(-5, -6, -3) = []
+[0, 1, 2].slice(-5, -6, -2) = []
+[0, 1, 2].slice(-5, -6, -1) = []
@@ -169,6 +169,6 @@
-[0, 1, 2].slice(-5, -5, -6) = [0]
-[0, 1, 2].slice(-5, -5, -5) = [0]
-[0, 1, 2].slice(-5, -5, -4) = [0]
-[0, 1, 2].slice(-5, -5, -3) = [0]
-[0, 1, 2].slice(-5, -5, -2) = [0]
-[0, 1, 2].slice(-5, -5, -1) = [0]
+[0, 1, 2].slice(-5, -5, -6) = []
+[0, 1, 2].slice(-5, -5, -5) = []
+[0, 1, 2].slice(-5, -5, -4) = []
+[0, 1, 2].slice(-5, -5, -3) = []
+[0, 1, 2].slice(-5, -5, -2) = []
+[0, 1, 2].slice(-5, -5, -1) = []
@@ -181,6 +181,6 @@
-[0, 1, 2].slice(-5, -4, -6) = [0]
-[0, 1, 2].slice(-5, -4, -5) = [0]
-[0, 1, 2].slice(-5, -4, -4) = [0]
-[0, 1, 2].slice(-5, -4, -3) = [0]
-[0, 1, 2].slice(-5, -4, -2) = [0]
-[0, 1, 2].slice(-5, -4, -1) = [0]
+[0, 1, 2].slice(-5, -4, -6) = []
+[0, 1, 2].slice(-5, -4, -5) = []
+[0, 1, 2].slice(-5, -4, -4) = []
+[0, 1, 2].slice(-5, -4, -3) = []
+[0, 1, 2].slice(-5, -4, -2) = []
+[0, 1, 2].slice(-5, -4, -1) = []
@@ -313,6 +313,6 @@
-[0, 1, 2].slice(-4, -6, -6) = [0]
-[0, 1, 2].slice(-4, -6, -5) = [0]
-[0, 1, 2].slice(-4, -6, -4) = [0]
-[0, 1, 2].slice(-4, -6, -3) = [0]
-[0, 1, 2].slice(-4, -6, -2) = [0]
-[0, 1, 2].slice(-4, -6, -1) = [0]
+[0, 1, 2].slice(-4, -6, -6) = []
+[0, 1, 2].slice(-4, -6, -5) = []
+[0, 1, 2].slice(-4, -6, -4) = []
+[0, 1, 2].slice(-4, -6, -3) = []
+[0, 1, 2].slice(-4, -6, -2) = []
+[0, 1, 2].slice(-4, -6, -1) = []
@@ -325,6 +325,6 @@
-[0, 1, 2].slice(-4, -5, -6) = [0]
-[0, 1, 2].slice(-4, -5, -5) = [0]
-[0, 1, 2].slice(-4, -5, -4) = [0]
-[0, 1, 2].slice(-4, -5, -3) = [0]
-[0, 1, 2].slice(-4, -5, -2) = [0]
-[0, 1, 2].slice(-4, -5, -1) = [0]
+[0, 1, 2].slice(-4, -5, -6) = []
+[0, 1, 2].slice(-4, -5, -5) = []
+[0, 1, 2].slice(-4, -5, -4) = []
+[0, 1, 2].slice(-4, -5, -3) = []
+[0, 1, 2].slice(-4, -5, -2) = []
+[0, 1, 2].slice(-4, -5, -1) = []
@@ -337,6 +337,6 @@
-[0, 1, 2].slice(-4, -4, -6) = [0]
-[0, 1, 2].slice(-4, -4, -5) = [0]
-[0, 1, 2].slice(-4, -4, -4) = [0]
-[0, 1, 2].slice(-4, -4, -3) = [0]
-[0, 1, 2].slice(-4, -4, -2) = [0]
-[0, 1, 2].slice(-4, -4, -1) = [0]
+[0, 1, 2].slice(-4, -4, -6) = []
+[0, 1, 2].slice(-4, -4, -5) = []
+[0, 1, 2].slice(-4, -4, -4) = []
+[0, 1, 2].slice(-4, -4, -3) = []
+[0, 1, 2].slice(-4, -4, -2) = []
+[0, 1, 2].slice(-4, -4, -1) = []

So there are still discrepancies but less than before.

I'm not sure if it's even possible to make the behavior be in par with the Python because of the clamping premise which is needed for the current default arg for end. Contrary to Python we use only ints as arguments, no None/null. 🤔

It might be a matter of leaving it as is and documenting it in detail.

AThousandShips · 2023-07-07T11:38:20Z

Will take a look as well, might be some solution

kleonc · 2023-07-07T11:43:21Z

Will take a look as well, might be some solution

After rethinking the solution could be to treat begin as is instead of always clamping it. Meaning when (begin < -array.size() and step < 0) or (begin >= array.size() and step > 0) it would always result in an empty array.

AThousandShips · 2023-07-07T12:16:15Z

Tested and that seems to work, will do some more structured testing when I'm able and add this change

Will also document how to get the last value, as it's not entirely intuitive, and some other details

kleonc

Seems like we're settled on the behavior, now the diff compared to Python is empty (so hopefully the behavior is more intuitive now). 👍

Now just a matter of updating the docs accordingly (+maybe some additional tests could be added).

core/variant/array.cpp

AThousandShips · 2023-07-07T13:00:27Z

For the documentation I'm thinking something on the lines of:

Note: To include the first element of the Array when step is negative, use -(size + 1) for end.

Or something along those lines, exact formatting and wording to be worked out

Edit: Alternatively

[b]Note:[/b] To include the first element when [param step] is negative, use [code]arr.slice(begin, -arr.size() - 1, 
step)[/code] (i.e. [code][0, 1, 2].slice(1, -4, -1)[/code] returns [code][1, 0][/code]).

kleonc · 2023-07-07T15:21:57Z

Regarding the docs I'd firstly get rid of mentioning any clamping:

godot/doc/classes/Array.xml

Line 583 in c3b0a92

    
           The absolute value of [param begin] and [param end] will be clamped to the array size, so the default value for [param end] makes it slice to the size of the array by default (i.e. [code]arr.slice(1)[/code] is a shorthand for [code]arr.slice(1, arr.size())[/code]).

Conceptually it doesn't do any clamping now, the indexes which would end up out of bounds are just skipped / not included (fact it uses clamping is implementation detail). So it should be sufficient to explain that:

Negative indices are relative to the end of the array.
end is exclusive.
Indices which would be out of bounds are skipped/ignored.

Also indeed should be worth explicitly pointing out that (for non-empty array) because of (1) and (2):

end = -1 can't be used as "before the first element" (when step < 0)(any end < -size can be used for this instead),
end = 0 can't be used as "after the last element" (when step > 0)(any end > size - 1 can be used for this instead).

I guess the quest is to write this in a clear and simple manner. 🙂 Some code examples could clarify things too.

Edit: Alternatively

[b]Note:[/b] To include the first element when [param step] is negative, use [code]arr.slice(begin, -arr.size() - 1, -
step)[/code] (i.e. [code][0, 1, 2].slice(1, -4, -1)[/code] returns [code][1, 0][/code]).

BTW if step is said to be negative then -step would be positive. 🙃

AThousandShips · 2023-07-07T15:23:15Z

Forgot to update my example here with -step lol, had done in my current attempt at doc

AThousandShips · 2023-07-07T21:17:03Z

Will add more to the documentation and see what additional tests to add tomorrow probably

YuriSizov · 2023-07-12T11:02:00Z

@AThousandShips What's the status on this? 🙃

AThousandShips · 2023-07-12T11:03:13Z

The documentation needs a tweak, but uncertain how to word it, otherwise I believe it's ready

Been unable to work on it the last few days

YuriSizov · 2023-07-12T11:05:10Z

You mentioned adding tests too?

AThousandShips · 2023-07-12T11:05:50Z

Added some, unsure what other cases need covering

@kleonc any thoughts?

YuriSizov

I agree that the documentation can be significantly improved, but let's leave it to a follow-up. Fixes that improve the implementation are good as is.

Stale review

YuriSizov · 2023-07-17T14:48:53Z

Thanks!

AThousandShips · 2023-07-17T14:49:37Z

Thank you!

And feel free to provide any feedback on the documentation, or add if yourself

YuriSizov · 2023-07-17T15:03:22Z

And feel free to provide any feedback on the documentation, or add if yourself

I mostly agree with @kleonc's assessment which points we need to hit, so it's a matter of presenting them in a readable format. I think the description as is can be hard to comprehend as it uses a very robotic, technical voice to describe the logic. Like, the second sentence is

The absolute value of [param begin] and [param end] will be clamped to the array size, so the default value for [param end] makes it slice to the size of the array by default

And it deals with many non-intuitive things (such as "absolute value", "clamping") when all the user wants to do is get a portion of an array. And the same happens with every sentence that follows.

If either [param begin] or [param end] are negative, they will be relative to the end of the array

Upon reading it I'm hard pressed to imagine what being relative to the end means.

If specified, [param step] is the relative index between source elements.

Step may be a relative index if you had to define it scientifically, but who talks like that? 🙃 Even changing that to talk about "every Nth element" would be better, though still not ideal.

I don't have a particular suggestion how to phrase it all better at the moment, though.

YuriSizov · 2023-08-31T17:50:10Z

Cherry-picked for 4.1.2.

AThousandShips added bug topic:core crash cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 6, 2023

AThousandShips added this to the 4.2 milestone Jul 6, 2023

AThousandShips requested review from a team as code owners July 6, 2023 13:16

akien-mga previously approved these changes Jul 6, 2023

View reviewed changes

kleonc previously requested changes Jul 6, 2023

View reviewed changes

AThousandShips marked this pull request as draft July 6, 2023 14:11

akien-mga requested a review from a team July 6, 2023 14:13

AThousandShips force-pushed the array_slice_range branch from dc5a8fe to 8fe32ec Compare July 6, 2023 14:38

AThousandShips marked this pull request as ready for review July 6, 2023 14:38

akien-mga requested a review from kleonc July 7, 2023 06:24

kleonc mentioned this pull request Jul 7, 2023

Impossible to get the last element of an array using negative indices in ARRAY.slice() #61530

Closed

AThousandShips force-pushed the array_slice_range branch from 8fe32ec to 42a6cdf Compare July 7, 2023 11:15

AThousandShips force-pushed the array_slice_range branch from 42a6cdf to 65df218 Compare July 7, 2023 12:17

kleonc reviewed Jul 7, 2023

View reviewed changes

core/variant/array.cpp Outdated Show resolved Hide resolved

core/variant/array.cpp Outdated Show resolved Hide resolved

AThousandShips force-pushed the array_slice_range branch from 65df218 to e7628f6 Compare July 7, 2023 21:16

AThousandShips requested a review from a team as a code owner July 7, 2023 21:16

Fix range error for Array.slice

7752a0d

AThousandShips force-pushed the array_slice_range branch from e7628f6 to 7752a0d Compare July 7, 2023 21:19

YuriSizov approved these changes Jul 17, 2023

View reviewed changes

YuriSizov removed the cherrypick:4.0 label Jul 17, 2023

YuriSizov merged commit 3d04a22 into godotengine:master Jul 17, 2023

AThousandShips deleted the array_slice_range branch July 17, 2023 14:49

YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix range error for `Array.slice` #79103

Fix range error for `Array.slice` #79103

AThousandShips commented Jul 6, 2023 •

edited

Loading

kleonc left a comment

AThousandShips commented Jul 6, 2023

AThousandShips commented Jul 6, 2023

AThousandShips commented Jul 6, 2023

kleonc commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

kleonc commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

kleonc commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

kleonc commented Jul 7, 2023 •

edited

Loading

AThousandShips commented Jul 7, 2023 •

edited

Loading

kleonc left a comment

AThousandShips commented Jul 7, 2023 •

edited

Loading

kleonc commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

YuriSizov commented Jul 12, 2023

AThousandShips commented Jul 12, 2023 •

edited

Loading

YuriSizov commented Jul 12, 2023

AThousandShips commented Jul 12, 2023

YuriSizov left a comment

YuriSizov commented Jul 17, 2023

AThousandShips commented Jul 17, 2023

YuriSizov commented Jul 17, 2023

YuriSizov commented Aug 31, 2023

Fix range error for Array.slice #79103

Fix range error for Array.slice #79103

Conversation

AThousandShips commented Jul 6, 2023 • edited Loading

kleonc left a comment

Choose a reason for hiding this comment

AThousandShips commented Jul 6, 2023

AThousandShips commented Jul 6, 2023

AThousandShips commented Jul 6, 2023

kleonc commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

kleonc commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

kleonc commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

kleonc commented Jul 7, 2023 • edited Loading

AThousandShips commented Jul 7, 2023 • edited Loading

kleonc left a comment

Choose a reason for hiding this comment

AThousandShips commented Jul 7, 2023 • edited Loading

kleonc commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

AThousandShips commented Jul 7, 2023

YuriSizov commented Jul 12, 2023

AThousandShips commented Jul 12, 2023 • edited Loading

YuriSizov commented Jul 12, 2023

AThousandShips commented Jul 12, 2023

YuriSizov left a comment

Choose a reason for hiding this comment

YuriSizov commented Jul 17, 2023

AThousandShips commented Jul 17, 2023

YuriSizov commented Jul 17, 2023

YuriSizov commented Aug 31, 2023

Fix range error for `Array.slice` #79103

Fix range error for `Array.slice` #79103

AThousandShips commented Jul 6, 2023 •

edited

Loading

kleonc commented Jul 7, 2023 •

edited

Loading

AThousandShips commented Jul 7, 2023 •

edited

Loading

AThousandShips commented Jul 7, 2023 •

edited

Loading

AThousandShips commented Jul 12, 2023 •

edited

Loading