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

Renamed Direction.idHorizontal and related methods #4036

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

Estecka
Copy link
Contributor

@Estecka Estecka commented Oct 27, 2024

The field Direction.idHorizontal doesn't make sense to interpret as an id, and it certainly doesn't correspond the id of any other direction. This field's value is very frequently multiplied by 90 to obtain an angle in degree, so it makes more sense to to see it as the number of horizontal quarter turns required to face this direction.

asRotation() does precisely that, so I renammed it to getPositiveHorizontalDegrees(). It does almost the same job as the existing getHorizontalDegrees(Direction), except that the later throws when fed a vertical direction, and returns angles wrapped between -90° and 180°. The "positive" counterpart returns angles wrapped between 0° and 270°.

I renamed getHorizontalDegrees to getHorizontalDegreesOrThrow to further emphasize the difference in behaviour.

@Octol1ttle
Copy link
Contributor

Since asRotation got renamed, maybe fromRotation should also be renamed?

@Estecka
Copy link
Contributor Author

Estecka commented Oct 31, 2024

Good spot, I hadn't noticed these.
Added fromRotation->fromHorizontalDegrees and fromHorizontal->fromHorizontalQuarterTurns

@@ -51,7 +51,7 @@ CLASS net/minecraft/class_2350 net/minecraft/util/math/Direction
ARG 1 y
ARG 2 z
METHOD method_10148 getOffsetX ()I
METHOD method_10150 fromRotation (D)Lnet/minecraft/class_2350;
METHOD method_10150 fromHorizontalDegrees (D)Lnet/minecraft/class_2350;
ARG 0 rotation
Copy link
Contributor

Choose a reason for hiding this comment

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

rename the argument as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I believe angle would suffice for this one in particular.

apple502j
apple502j previously approved these changes Nov 6, 2024
@modmuss50 modmuss50 added the update-base Add this label to a pull request to automatically change the target branch to the default branch. label Nov 13, 2024
@github-actions github-actions bot changed the base branch from 1.21.3 to 24w46a November 13, 2024 18:15
@github-actions github-actions bot dismissed apple502j’s stale review November 13, 2024 18:15

The base branch was changed.

Copy link
Contributor

🚀 Target branch has been updated to 24w46a

@github-actions github-actions bot added snapshot A PR that targets a snapshot version of Minecraft and removed update-base Add this label to a pull request to automatically change the target branch to the default branch. labels Nov 13, 2024
@modmuss50 modmuss50 merged commit d2291f4 into FabricMC:24w46a Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
snapshot A PR that targets a snapshot version of Minecraft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants