Add more array methods: find_custom, rfind_custom

[SlashScreen]

Added 3 new array functions - count_custom, first_custom, and last_custom. These perform the functions of the existing count,, first, and rfind respectively, but using a Callable as the conditions, like sort_custom.
Last could be renamed rfind_custom for consistency sake.

• Bugsquad edit, closes: Add more array methods: first_custom, last_custom, count_custom godot-proposals#6889

[AThousandShips]

Please fix the message of your commit

[AThousandShips]

These need to be added to the binds for them to be useful in scripting

count_custom seems excessive, can easily be worked around with reduce (the same argument that rejected adding min/max_custom, in favor of documentation)

[AThousandShips]

This name feels misleading, using first makes it sound like it returns that value, not the index, just like suggested on the previous PR I'd say find_custom and rfind_custom

[SlashScreen]

You're right, I didn't think about using reduce in that way. I can remove that. Should I rename last_custom to rfind_custom?
As for the first ambiguity, that too is correct, but does saying find_custom make it sound like it does the same thing as any? Or no, because any returns a bool.
Also, yeah, not sure how I thought it would work without binding. I couldn't find where it was bound before, but after another look, I found it.

[dalexeev]

Should I rename last_custom to rfind_custom?

I think it should be find_custom and rfind_custom, not first_custom and last_custom, for consistency with the already existing naming scheme.

I couldn't find where it was bound before, but after another look, I found it.

godot/core/variant/variant_call.cpp

Lines 2204 to 2208 in 237bd0a

	/* Array */
	bind_method(Array, size, sarray(), varray());
	bind_method(Array, is_empty, sarray(), varray());
	bind_method(Array, clear, sarray(), varray());

Don't forget the docs (XML files in the doc/classes folder). You can use bin/<godot_executable> --doctool to generate an empty method descriptions and fill it later.

Also, you should consider adding tests for the methods.

[SlashScreen]

How would I create a callable for the tests? I don't see precedent for this, though I could be missing something.

[dalexeev]

How would I create a callable for the tests?

I think you can use callable_mp(&object, &Class::method):

godot/tests/core/object/test_object.h

Line 351 in 237bd0a

target.connect("my_custom_signal", callable_mp(&object, &Object::notify_property_list_changed));

or callable_mp_static(&Class::static_method):

godot/tests/core/threads/test_worker_thread_pool.h

Lines 46 to 48 in 237bd0a

	static void static_callable_test() {
	counter[0].sub(2);
	}

godot/tests/core/threads/test_worker_thread_pool.h

Line 63 in 237bd0a

tasks2[i] = WorkerThreadPool::get_singleton()->add_task(callable_mp_static(static_callable_test), !low_priority);

[AThousandShips]

Or add it as a gdscript test under modules/gdscript/tests/scripts/runtime/features

[dalexeev]

Or add it as a gdscript test under modules/gdscript/tests/scripts/runtime/features

I think it's less preferable, GDScript tests are for testing the GDScript implementation, not the core.

[SlashScreen]

How would I create a callable for the tests?

I think you can use callable_mp(&object, &Class::method):

godot/tests/core/object/test_object.h

Line 351 in 237bd0a

target.connect("my_custom_signal", callable_mp(&object, &Object::notify_property_list_changed));

or callable_mp_static(&Class::static_method):

godot/tests/core/threads/test_worker_thread_pool.h

Lines 46 to 48 in 237bd0a

	static void static_callable_test() {
	counter[0].sub(2);
	}

godot/tests/core/threads/test_worker_thread_pool.h

Line 63 in 237bd0a

tasks2[i] = WorkerThreadPool::get_singleton()->add_task(callable_mp_static(static_callable_test), !low_priority);

Ah, so I can use a c++ function as a callable?

[dalexeev]

Ah, so I can use a c++ function as a callable?

Yes.

[SlashScreen]

I hope I amended it correctly.
I added the test, removed count custom, added some documentation, and added some tests.

[YuriSizov]

Please also make sure that the commit message refers to the actual names of the methods.

Personally, I agree that these should be variants of find. first_custom and last_custom don't make sense semantically. So the current state of this PR seems fine to me. Provided we agree to add these at all, of course.

[SlashScreen]

Fixed typo and comment style nitpick.

[SlashScreen]

If there's any more changes that need be made, I will address them tomorrow, as it is 3:30 in the morning and my brain is shutting down.
Could someone double-check that I created the callable properly in the tests?

[RedMser]

The method documentation should include that the func callable only gets called with one parameter: once for each element in the array. See the documentation of map, since I think it's a lot clearer.
E.g. you could otherwise assume it also includes the index as a second parameter or something (which I kinda wish for, but this needs a separate proposal and PR).

[SlashScreen]

Resolved typos and style checks from @AThousandShips .

[SlashScreen]

Whoops, I messed up the test. That's what I get for having nvim in a thin window where I can't see the whole line.

[YuriSizov]

You still need to amend the commit message to reflect what the methods are actually called.

[SlashScreen]

nice catch.

[AThousandShips]

Looks good save for some corrections to the documentation and style

[SlashScreen]

Integrated the changes, @AThousandShips .

[AThousandShips]

Turns out there's already a PR for this change, predating it:

• Add Array.find_if()/rfind_if() methods #73490

But this one is more refined IMO

[SlashScreen]

oh dear, I'm not sure i updated the documentation for the other function... Will the checks need to be run again?

[AThousandShips]

They do, but no worries just go ahead and fix them

[maximkulkin]

This one is exactly same as #73490 minus naming with an exception that the other one has much more comprehensive test suite.

[SlashScreen]

So is mine redundant?

[AThousandShips]

Will be a decision which one to merge

[SlashScreen]

Changed semantics on rfind_custom().

[AThousandShips]

Reviewers, see also:

• Add functions Array::find_first() and Array::get_first() #70727
• Add Array.find_if()/rfind_if() methods #73490

[Calinou]

• Superseded by Add callable support for find and rfind Array methods #95449.