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

chore: remove finite check from LocalPlayer #4662

Merged
merged 9 commits into from
Jun 19, 2021

Conversation

pollend
Copy link
Member

@pollend pollend commented May 12, 2021

we've mentioned in the past having a situation where the player position is placed at (0,0,0). I'm suspecting it has something to do with this logic. so for an invalid position we just return dest or a new Vector3/Quaternion. we shouldn't be just defaulting to 0,0,0. this ends up hiding a larger underlying problem where we see some odd behavior but we've written this logic in a way to hide a large problem. I rather have the game crash with a stacktrace then try poking at thing in the dark because we've written ourselves into a corner.

#4152

@pollend pollend marked this pull request as draft May 13, 2021 04:15
@github-actions github-actions bot added the Type: Chore Request for or implementation of maintenance changes label May 16, 2021
@pollend pollend marked this pull request as ready for review June 7, 2021 02:06
Copy link
Member

@jdrueckert jdrueckert left a comment

Choose a reason for hiding this comment

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

I agree that we don't want to hide any bigger problem with that but maybe instead of removing null checks entirely, we could consider throwing a reasonable exception instead.

@jdrueckert jdrueckert requested a review from skaldarnar June 15, 2021 21:20
Copy link
Member

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

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

I think not having explicit checks for the one-line methods getPosition, getRotation, and getVelocity is completely fine. The only thing we gain here is a user-readable message, but an NPE already pins it down to a single point of failure.

A similar argument holds for getViewPosition and getViewRotation. The line in which the NPE would occur indicates well enough what the issue is (at least to the devs).

We might add todo items to revisit this when we have better exception handling (soft and hard errors for crashing to menu or to desktop).

@jdrueckert jdrueckert merged commit ac62052 into develop Jun 19, 2021
@jdrueckert jdrueckert deleted the chore/remove-checks-LocalPlayer branch June 19, 2021 16:26
jdrueckert added a commit that referenced this pull request Jun 24, 2021
@pollend pollend restored the chore/remove-checks-LocalPlayer branch June 25, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Chore Request for or implementation of maintenance changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants