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 bsearch and bsearch_custom to Array #12590

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Conversation

poke1024
Copy link
Contributor

@poke1024 poke1024 commented Nov 2, 2017

See #12558

Pretty self explanatory, I guess. The main question is how to approach the API.

Samples:

var a = [1, 10, 50, 100, 120]
a.bsearch(40)
var a = [1, 10, 40, 40, 40, 50, 100, 120]
a.bsearch(40, false) # bisect from right
var a = [Vector2(5, 20), Vector2(-5, 30), Vector2(10, 100), Vector2(0, 120)]
a.bsearch_custom(Vector2(100, 50), self, "less_y")

The implementation is Python's time-proven bisect_left and bisect_right (https://hg.python.org/cpython/file/tip/Lib/bisect.py).

Not sure why the docs won't update.

@poke1024 poke1024 changed the title Add bsearch and bsearch_complex to Array Add bsearch and bsearch_custom to Array Nov 2, 2017
@poke1024 poke1024 force-pushed the bsearch branch 2 times, most recently from 81479b8 to 11cb374 Compare November 2, 2017 19:39
@groud groud added this to the 3.0 milestone Nov 3, 2017
@akien-mga
Copy link
Member

Reviewed on IRC and it's good to merge, it just needs a rebase to fix the merge conflict.

@vnen
Copy link
Member

vnen commented Nov 20, 2017

I find this awesome, but I'm not fond of the naming. EDIT: To clarify, bsearch sounds odd to me. But I noticed that C++ also has this function, so I guess it might be okay, though still strange for those not familiar with the terms.

@akien-mga
Copy link
Member

To clarify, bsearch sounds odd to me. But I noticed that C++ also has this function, so I guess it might be okay, though still strange for those not familiar with the terms.

I agree that for me, bisect_left and bisect_right would sound more familiar than bsearch, but I don't have a strong opinion either way. As long as it's well documented, it should be fine :)
We can always change the API later with a deprecation warning for the old name if it proves unclear to users.

@akien-mga akien-mga merged commit 1c2782a into godotengine:master Nov 21, 2017
@HummusSamurai
Copy link
Contributor

HummusSamurai commented Nov 21, 2017

@akien-mga I think bisect-left and -right sounds as if it splits the array in half. ("Bisects" it in the original highschool geometry sense of the word)

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.

5 participants