Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PackedByteArray, Array slice end exclusive, rename PackedByteArray subarray to slice #35901

Conversation

nathanfranke
Copy link
Contributor

@nathanfranke nathanfranke commented Feb 4, 2020

  • slice end index is now exclusive
  • Packed*Array::subarray is now Packed*Array::slice
func _ready() -> void:
	var array := PackedByteArray([0, 1, 2, 3, 4, 5, 6, 7])
	
	aprint(array.slice(0, 4))             ### 0, 1, 2, 3
	# Lines below would have crashed
	aprint(array.slice(4, 8))             ### 4, 5, 6, 7
	aprint(array.slice(0, array.size()))   ### 0, 1, 2, 3, 4, 5, 6, 7

func aprint(array: PackedByteArray) -> void:
	var s := String()
	for i in array:
		s += ", " + str(i)
	print(s.substr(2))

@aaronfranke
Copy link
Member

I'm curious why it was inclusive in the first place. This was added in #31172, pinging @creikey.

@creikey
Copy link
Contributor

creikey commented Feb 4, 2020

@aaronfranke I don't remember specifically why I did that in the first place, I should've written that down in the PR, but my guess is that it was because the pool byte array was already inclusive.

@nathanfranke nathanfranke force-pushed the pool-byte-array-subarray-exclusive branch from 528d377 to 4a8a4e8 Compare February 13, 2020 04:17
@nathanfranke
Copy link
Contributor Author

Updated to new master branch. Should work completely fine but additional testing may be needed

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Feb 25, 2020

I am assuming I have to rebase this since PoolByteArray changed to PackedByteArray. I should be able to do it tonight

Edit: Done with this!

@nathanfranke nathanfranke changed the title Make PoolByteArray and Array slice methods' end indexes exclusive like Python PackedByteArray, Array slice end exclusive, rename PackedByteArray subarray to slice Feb 25, 2020
@nathanfranke nathanfranke force-pushed the pool-byte-array-subarray-exclusive branch from 4a8a4e8 to d2fba1b Compare February 25, 2020 23:16
@creikey
Copy link
Contributor

creikey commented Apr 6, 2020

I remember why I made the array slice inclusive on both ends now, it's to make it so that when the p_step parameter is negative ( you're going backwards in the array ) you can use the array's size on the end bound and have it still work, it's symmetrical going forwards and backwards with the array indexes.

In hindsight, it might be more clear to the user if the first index and last index were switched around when p_step is negative, such that it starts at the end index and ends at the first index ( I think the behavior is opposite that right now ).

@madmiraal madmiraal mentioned this pull request May 2, 2020
@nathanfranke nathanfranke force-pushed the pool-byte-array-subarray-exclusive branch 2 times, most recently from dfe74f0 to 66129f9 Compare July 9, 2020 09:22
@nathanfranke
Copy link
Contributor Author

I updated this to master. I need to understand exactly what the expected behaviour should be, especially with negative steps.

Examples of current behaviour:

var a = [0, 1, 2, 3, 4, 5, 6, 7]

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

@nathanfranke nathanfranke force-pushed the pool-byte-array-subarray-exclusive branch 2 times, most recently from 2db1348 to 1fccef3 Compare October 29, 2020 10:24
@nathanfranke
Copy link
Contributor Author

Rebased again. Added documentation for PackedColorArray, PackedFloat32Array, PackedFloat64Array, PackedInt32Array, PackedInt64Array, PackedStringArray, PackedVector2Array, PackedVector3Array

@nathanfranke nathanfranke force-pushed the pool-byte-array-subarray-exclusive branch 2 times, most recently from 86df8b6 to d67dc38 Compare October 30, 2020 10:40
@Anutrix
Copy link
Contributor

Anutrix commented Oct 29, 2021

Requesting rebase.

@nathanfranke nathanfranke force-pushed the pool-byte-array-subarray-exclusive branch 2 times, most recently from abdec0c to 9d2d8b2 Compare November 27, 2021 02:44
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me 👍

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR meeting: makes sense, approved :)

@akien-mga akien-mga merged commit 46d3840 into godotengine:master Dec 7, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

See #56661 for a follow-up discussion on the behavior for negative end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants