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 client crash during final killcam #900

Merged
merged 7 commits into from
Nov 18, 2024

Conversation

Bobbyperson
Copy link
Contributor

@Bobbyperson Bobbyperson commented Nov 14, 2024

All this PR does is add cl_screenfade.gnut from basegame to Northstar. It includes a fix at lines 250 and 252. This PR fixes a bug affecting Infection and possibly other gamemodes that causes a client crash during final killcam. The bug in question along with a detailed writeup can be found here Bobbyperson/InfectionRework#7 (this issue does affect base infection).
384656045-4d8a67dd-b6c1-404f-9973-ff313e6b323d

Code review:

The added code is essentially copied from Vanilla code a few lines above.

Testing:

Testing the fix in question is quite difficult, as the bug is fairly random and hard to produce. However, because this same exact check is done just a few lines earlier and seems to be unintentionally missing, it shouldn't require any testing.

@github-actions github-actions bot added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Nov 14, 2024
@GeckoEidechse GeckoEidechse added the commits vanilla file For PRs that commit vanilla files from VPKs label Nov 14, 2024
@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Nov 14, 2024
@GeckoEidechse GeckoEidechse removed merge conflicts Blocked by merge conflicts, waiting on the author to resolve commits vanilla file For PRs that commit vanilla files from VPKs labels Nov 14, 2024
@GeckoEidechse
Copy link
Member

Added the corresponding vanilla file to main and updated this PR so it only reflects the actual change.

@Bobbyperson Bobbyperson changed the title feat: add cl_screenfade.gnut with fix fix: client crash during final killcam Nov 15, 2024
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code looks good, I don't think this really needs testing tbh. It's uhhh just an error check

@ASpoonPlaysGames ASpoonPlaysGames added READY TO MERGE This mergeable right now and removed needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Nov 18, 2024
Copy link
Contributor

@Zanieon Zanieon left a comment

Choose a reason for hiding this comment

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

Simple enough to understand, probably no need to test due to this simplicity.

Good to go for me.

@GeckoEidechse GeckoEidechse changed the title fix: client crash during final killcam Fix client crash during final killcam Nov 18, 2024
@GeckoEidechse GeckoEidechse merged commit d941fc3 into R2Northstar:main Nov 18, 2024
3 checks passed
@Bobbyperson Bobbyperson deleted the add-screenfade branch November 18, 2024 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants