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

Fix crash in l_body_get_ground_position if there is no body to get po… #5665

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

JonBooth78
Copy link
Contributor

…sition relative to

Every time I try to hyperspace I was crashing here (jumping from a space station, if that matters). Well the crash happened in GetAltitudeRelTo which was being passed a null body as the first argument.

@Max5377 , I think this relates to your earlier PR.

@Max5377
Copy link
Contributor

Max5377 commented Nov 18, 2023

I haven't considered that this can happen when doing PR. Thanks for spotting this.
P.S.: Maybe the better way would be to return null in Frame::GetFrame if no body is in this frame?
Update: @JonBooth78, additionally added null check for relTo in Body::GetAltitudeRelTo, in this commit.
Can you cherry-pick this commit into your branch?
Update2: another null check for frameBody in Ship:GetGPS.
This should be enough to fix this and additional errors that may appear later.

@JonBooth78
Copy link
Contributor Author

Hey @Max5377 , thanks for looking at this.

I think a frame reference with no body attached is fine and so we probably should still return that frame.

I'm not really across all the details here so if you'd like to progress this, how about you create a PR from your branch and then I'll close this one?

@Max5377
Copy link
Contributor

Max5377 commented Nov 18, 2023

@JonBooth78 cherry-pick is a command that basically allows you to copy someone else's commit into your branch. https://docs.github.com/en/desktop/managing-commits/cherry-picking-a-commit-in-github-desktop
https://git-scm.com/docs/git-cherry-pick
https://www.atlassian.com/git/tutorials/cherry-pick

You can also give write access to your repo by doing this
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/managing-teams-and-people-with-access-to-your-repository
so I can quickly change your commit.
I would modify this PR directly, but only mainteiners can do that.

I could also do PR to your branch, but I don't know if this would cause merge conflicts, because from the looks you haven't synced your fork in a long time.
I don't really want to do additional PR since, you've already done one that addresses this.

1. Added null check for relTo in Body::GetAltitudeRelTo;
2. Added null check for frameBody in Ship:GetGPS.

Co-Authored-By: JonBooth78 <249391+jonbooth78@users.noreply.github.com>
(cherry picked from commit 2cfbb42)
@JonBooth78
Copy link
Contributor Author

Hey @Max5377,I know how to pull more of your changes into this PR and I've done it for now. I'm just a bit concerned if there are more changes at code review, I might slow things down :-)

If you wanted to do a PR into my repo, you wouldn't want to do it to my master as yeah, that's a bit behind (and ahead), instead you'd want to do a PR to the source branch of this PR. Still that would leave me possibly in the middle. We'll see how we go.

Comment on lines 672 to 676
if (!astro)
{
lua_pushnil(l);
return 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

As a nitpick for both @JonBooth78 and @Max5377 - Pioneer's codebase prefers the use of starting braces on the same line for if and for statements, as well as class/struct definitions and single-line function definitions. Multi-line function definitions should (assuming I haven't forgotten anything) be the only structure with their beginning brace on a newline.

The clang-format CI check would be complaining at you over this... if it hadn't been silently broken for quite some time and I didn't notice. 😐

@JonBooth78
Copy link
Contributor Author

Ahh, man sorry @Web-eWorks , tidied those up for you now :-)

I'll also update my VS pro settings to remind me!

Mostly tidy up open scope braces to be on the same line (except for functions)
@Web-eWorks Web-eWorks merged commit 1abf8b9 into pioneerspacesim:master Nov 20, 2023
4 checks passed
@JonBooth78 JonBooth78 deleted the hyperpace-crash branch February 3, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants