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

Expose max_axis_index and max_axis_index for Vector2(i) #34005

Merged
merged 1 commit into from
Dec 6, 2021

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Nov 29, 2019

EDIT 3: This also renames them to have _index at the end as per @reduz's suggestion.

Also some cleanup with core's Vector3 methods so that it is consistent, for example it returns enums internally (GDScript still gets ints).

EDIT: Also now fixes #34190.

EDIT 2: max_axis and min_axis were added to Vector2(i) internally with #48629, but this PR exposes them and uses the enum as the type.

@aaronfranke aaronfranke requested review from neikeq and a team as code owners November 29, 2019 22:25
@Chaosus Chaosus added this to the 4.0 milestone Nov 30, 2019
@aaronfranke aaronfranke force-pushed the minmax branch 2 times, most recently from 2fdbb0c to 3d01712 Compare June 3, 2020 14:33
@aaronfranke aaronfranke force-pushed the minmax branch 3 times, most recently from 5df7338 to 2b18e80 Compare August 1, 2021 17:11
@aaronfranke aaronfranke changed the title Add MaxAxis and MinAxis to Vector2 Add MaxAxis and MinAxis to Vector2(i) Aug 26, 2021
@aaronfranke aaronfranke changed the title Add MaxAxis and MinAxis to Vector2(i) Expose max_axis and min_axis for Vector2(i) Aug 26, 2021
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.

Something is not working as the added enums don't appear in the docs and are not documented as return types.

@aaronfranke
Copy link
Member Author

@akien-mga I'm not sure if that's possible to do with the current code for Variant types bound in variant_call.cpp. All other places in Godot that have enums as return types are classes that use ClassDB::bind_method.

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.

Reviewed in PR meeting: fine to expose, but we should see about better naming for this both here and for Vector3i

Reduz suggestion:
get_min_axis_index and get_max_axis_index

@aaronfranke aaronfranke changed the title Expose max_axis and min_axis for Vector2(i) Expose max_axis_index and max_axis_index for Vector2(i) Dec 3, 2021
Some cleanup with Vector3(i)'s methods so that it is consistent with Vector2, for example it returns enums internally (GDScript still gets ints).
@aaronfranke
Copy link
Member Author

Done, but I used the names max_axis_index and max_axis_index instead of having get_ because none of the other Vector2/2i/3/3i methods start with get_. Ex: We have length not get_length.

@akien-mga akien-mga merged commit 5baf20e into godotengine:master Dec 6, 2021
@akien-mga
Copy link
Member

Thanks!

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.

max_axis() and min_axis() - intended behaviour?
5 participants