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

Add sort and has methods to PackedArrays #32144

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Sep 15, 2019

This PR adds two methods, for sorting using Godot's built-in sorting system, and for checking if it contains a specific value.

The methods are added for PackedByteArray, PackedColorArray, PackedInt32Array, PackedInt64Array, PackedFloat32Array, PackedFloat64Array, PackedStringArray, PackedVector2Array, and PackedVector3Array. I've exposed the methods to Variant and GDScript, but I also need this functionality internally for #31171

@Xrayez
Copy link
Contributor

Xrayez commented Sep 15, 2019

Perhaps rename contains to has to be consistent with Arrays respective method?

@aaronfranke
Copy link
Member Author

aaronfranke commented Sep 15, 2019

@Xrayez Thanks for pointing it out, but it might make more sense to rename has to contains in 4.0. EDIT: Many people seem to prefer has so I changed this PR to have has.

@aaronfranke aaronfranke force-pushed the poolarray branch 2 times, most recently from 850a58a to 0904f5f Compare September 17, 2019 22:59
@akien-mga
Copy link
Member

but it might make more sense to rename has to contains in 4.0.

That's a separate discussion, but if you want this change in 3.2, consistency is more important than potential future changes. Both Array and Dictionary use has, so there's no reason for PoolArrays not to.

@reduz
Copy link
Member

reduz commented Sep 25, 2019

This PR really overcomplicated things, simply

  1. copy the sort functions (sort() and sort_custom()) functions from vector.h
  2. do sorting with a locked poolvector, else it will be super slow.

@akien-mga akien-mga added this to the 4.0 milestone Sep 25, 2019
@aaronfranke aaronfranke changed the title Add sort and contains methods to PoolArrays Add sort and has methods to PoolArrays Oct 2, 2019
@nathanfranke
Copy link
Contributor

Just a note:
core/sort_array.h already contains heap sort and insertion sort
std::sort uses QuickSort already (typically)

@akien-mga akien-mga requested a review from reduz October 27, 2019 08:52
@aaronfranke aaronfranke force-pushed the poolarray branch 2 times, most recently from f354233 to 5d00b10 Compare January 17, 2020 03:01
@aaronfranke aaronfranke changed the title Add sort and has methods to PoolArrays Add sort and has methods to PackedArrays Feb 21, 2020
@aaronfranke
Copy link
Member Author

aaronfranke commented Feb 21, 2020

Because of the removal of PoolArrays, I basically had to recreate this PR from scratch. Fortunately, it was super simple. Since Vector<T> already has a sort method, all I had to do is expose it, and implement/expose the has method as well (amazingly, a copy-paste with no changes worked).

Since this PR is so simple and I need it for another PR, this should probably be merged soon-ish.

@aaronfranke
Copy link
Member Author

Updated again, now compatible with Packed(Int32/Int64/Float32/Float64)Arrays.

core/vector.h Outdated
Comment on lines 95 to 97
bool has(const T &p_val) {
for (int i = 0; i < size(); i++) {
if (get(i) == p_val) {
return true;
}
}
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Array::has() relies on find(), which is also implemented for Vector/CowData. Maybe it would be faster than this? (Didn't try.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure which is faster, but they both look similar, and it would be simpler and better for consistency with Array to use find, so I changed it to find.

@@ -2182,6 +2216,8 @@ void register_variant_methods() {
ADDFUNC1(PACKED_COLOR_ARRAY, NIL, PackedColorArray, remove, INT, "idx", varray());
ADDFUNC2R(PACKED_COLOR_ARRAY, INT, PackedColorArray, insert, INT, "idx", COLOR, "color", varray());
ADDFUNC1(PACKED_COLOR_ARRAY, NIL, PackedColorArray, resize, INT, "idx", varray());
ADDFUNC1R(PACKED_COLOR_ARRAY, BOOL, PackedColorArray, has, COLOR, "value", varray());
ADDFUNC0(PACKED_COLOR_ARRAY, NIL, PackedColorArray, sort, varray());
Copy link
Member

Choose a reason for hiding this comment

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

Since PackedVectors are code types new methods should be added to the GDNative API as well: gdnative_api.json, packed_arrays.cpp, packed_arrays.h

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the info, done.

@akien-mga akien-mga merged commit 624eff4 into godotengine:master Jul 7, 2020
@akien-mga
Copy link
Member

Thanks!

@Calinou
Copy link
Member

Calinou commented Aug 31, 2020

Is it possible to remake this pull request for the 3.2 branch in a fully compatible manner?

@aaronfranke
Copy link
Member Author

I made this before 3.2 was released, but Akien gave it the 4.0 milestone.

@akien-mga
Copy link
Member

I made this before 3.2 was released, but Akien gave it the 4.0 milestone.

Yes, because it was made during feature freeze, and so wouldn't be merged during the stage/RC stage for 3.2. A PR gets the milestone matching the version which will be in development when it's meant to be merged in master. That doesn't mean that it can't be considered for a backport when we have no hurry and can take the time to assess and test changes like this one (as you've seen with 3.2.x releases which got hundreds of backports).

kdiduk added a commit to kdiduk/godot that referenced this pull request Jul 7, 2022
This is a backport from 4.0 to 3.x that adds 'sort' and 'has' methods
to the following types:
- PoolByteArray
- PoolIntArray
- PoolRealArray
- PoolStringArray
- PoolVector2Array
- PoolVector3Array
- PoolColorArray

For all the types above, the methods 'sort' and 'has' have been
exposed to the GDNative API (v. 1.3)

Since the method 'has' was already implemented in GDScript before,
in this commit it has been only exposed to GDNative API.

The classes documentation is updated.

The method 'sort' uses the exisging class "Sorter".

Pooled arrays in 4.0 are rewritten, that's why this backport is not
completely indentical to the original PR made for 4.0 (see godotengine#32144).
Riordan-DC pushed a commit to Riordan-DC/godot that referenced this pull request Jan 24, 2023
This is a backport from 4.0 to 3.x that adds 'sort' and 'has' methods
to the following types:
- PoolByteArray
- PoolIntArray
- PoolRealArray
- PoolStringArray
- PoolVector2Array
- PoolVector3Array
- PoolColorArray

For all the types above, the methods 'sort' and 'has' have been
exposed to the GDNative API (v. 1.3)

Since the method 'has' was already implemented in GDScript before,
in this commit it has been only exposed to GDNative API.

The classes documentation is updated.

The method 'sort' uses the exisging class "Sorter".

Pooled arrays in 4.0 are rewritten, that's why this backport is not
completely indentical to the original PR made for 4.0 (see godotengine#32144).
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.

8 participants