Optimize Vector Object Usage#2312
Open
Axionize wants to merge 23 commits into
Open
Conversation
Contributor
Author
|
@ManInMyVan can I get a review? |
ManInMyVan
requested changes
Oct 22, 2025
ManInMyVan
requested changes
Oct 22, 2025
Contributor
|
please address my 27 comments before I review this again |
… CheckManagerListener
# Conflicts: # common/src/main/java/ac/grim/grimac/utils/data/ReachInterpolationData.java
Axionize
force-pushed
the
optimize/vector-objects
branch
from
a763af1 to
cad28d6
Compare
Contributor
Author
|
@ManInMyVan I believe I've addressed all your existing code review concerns. |
ManInMyVan
requested changes
Oct 30, 2025
Comment on lines
+498
to
+500
| // Previously we called Vector3dm.add(player.likelyExplosions.vector) after checking (player.firstBreadExplosion != null) | ||
| // which would've thrown an NPE if player.likelyExplosions could be null here, we can ignore this | ||
| // empirically this means when firstBreadExplosion != null then likelyExplosions is also != null |
Contributor
There was a problem hiding this comment.
I don't really like comments referencing removed code, can you reword this?
Contributor
Author
There was a problem hiding this comment.
I can't think of a way how to explain this effectively without it...
Contributor
There was a problem hiding this comment.
you could just skip the explanation and say it shouldn't be null
Comment on lines
951
to
954
| public Vector3dm getMovementResultFromInput(GrimPlayer player, double x, double y, double z, float f, float f2) { | ||
| float f2InRadians = GrimMath.radians(f2); | ||
| float f3 = player.trigHandler.sin(f2InRadians); | ||
| float f4 = player.trigHandler.cos(f2InRadians); |
Contributor
There was a problem hiding this comment.
might as well deobf aswell
Suggested change
| public Vector3dm getMovementResultFromInput(GrimPlayer player, double x, double y, double z, float f, float f2) { | |
| float f2InRadians = GrimMath.radians(f2); | |
| float f3 = player.trigHandler.sin(f2InRadians); | |
| float f4 = player.trigHandler.cos(f2InRadians); | |
| public Vector3dm getMovementResultFromInput(GrimPlayer player, double x, double y, double z, float speed, float yaw) { | |
| float yawRadians = GrimMath.radians(yaw); | |
| float yawSin = player.trigHandler.sin(yawRadians); | |
| float yawCos = player.trigHandler.cos(yawRadians); |
Contributor
Author
There was a problem hiding this comment.
If you're certain these are correct I'll apply them
Contributor
There was a problem hiding this comment.
it's based on a usage's inputs
Comment on lines
+31
to
+34
| vectorData.vectorX *= swimmingFriction; | ||
| vectorData.vectorY *= 0.8F; | ||
| vectorData.vectorZ *= swimmingFriction; | ||
| vectorData.vectorY = FluidFallingAdjustedMovement.getAdjustedY(player, playerGravity, isFalling, vectorData.vectorY); |
Contributor
There was a problem hiding this comment.
Suggested change
| vectorData.vectorX *= swimmingFriction; | |
| vectorData.vectorY *= 0.8F; | |
| vectorData.vectorZ *= swimmingFriction; | |
| vectorData.vectorY = FluidFallingAdjustedMovement.getAdjustedY(player, playerGravity, isFalling, vectorData.vectorY); | |
| vectorData.vectorX *= swimmingFriction; | |
| vectorData.vectorY = FluidFallingAdjustedMovement.getAdjustedY(player, playerGravity, isFalling, vectorData.vectorY * 0.8F); | |
| vectorData.vectorZ *= swimmingFriction; |
Comment on lines
+186
to
+187
| data.vectorX = data.vectorX + player.trigHandler.sin(GrimMath.radians(-player.yaw)) * f; | ||
| data.vectorZ = data.vectorZ + (double) (player.trigHandler.cos(GrimMath.radians(player.yaw)) * f); |
Contributor
There was a problem hiding this comment.
Suggested change
| data.vectorX = data.vectorX + player.trigHandler.sin(GrimMath.radians(-player.yaw)) * f; | |
| data.vectorZ = data.vectorZ + (double) (player.trigHandler.cos(GrimMath.radians(player.yaw)) * f); | |
| data.vectorX += player.trigHandler.sin(GrimMath.radians(-player.yaw)) * f; | |
| data.vectorZ += (double) (player.trigHandler.cos(GrimMath.radians(player.yaw)) * f); |
Comment on lines
+183
to
+185
| // TODO (Mutation) (Confirm if Correct) | ||
| // It seems possible for applyStuckSpeed = 0, and 1 to add the same vector to the set twice | ||
| // since we're mutating data.vectorX, Y, Z which is very likely inorrect but I don't know enough to say for certain |
Contributor
There was a problem hiding this comment.
we could check if the player has a stuck speed before looping both if that's the issue
Contributor
Author
There was a problem hiding this comment.
The problem is that the data reference is the same, so if you modify data.vectorX it'll be applied to both instances of data in the list
Contributor
There was a problem hiding this comment.
you could make VectorData cloneable and add this before you modify it
if (!player.isForceStuckSpeed()) {
data = data.clone();
}
Contributor
Author
|
I was wondering if there’s an update on whether this PR is expected to be merged, it could be a nice optimization. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace swathes of unnecessary object allocation with faster primitive logic and other minor optimizations