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

Add a safeguard to map command #529

Merged
merged 9 commits into from
Oct 2, 2023

Conversation

catornot
Copy link
Member

@catornot catornot commented Aug 2, 2023

adds safeguard to the map command that prevents it from executing if the map is invalid.

I wanted to implement this originally as a plugin but I think it might be quite useful for everyone.

image

NorthstarDLL/util/printmaps.cpp Outdated Show resolved Hide resolved
NorthstarDLL/util/printmaps.cpp Outdated Show resolved Hide resolved
NorthstarDLL/util/printmaps.cpp Outdated Show resolved Hide resolved
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.

Would be nice to get this merged, just a couple of questions/suggestions

NorthstarDLL/util/printmaps.cpp Outdated Show resolved Hide resolved
NorthstarDLL/util/printmaps.cpp Outdated Show resolved Hide resolved
NorthstarDLL/util/printmaps.cpp Outdated Show resolved Hide resolved
@catornot catornot requested a review from Alystrasz August 4, 2023 00:31
@catornot catornot added the needs testing Changes from the PR still need to be tested label Aug 4, 2023
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Game does not crash anymore when passed an invalid map name, and prints a nice warning message instead.

F1F7Y
F1F7Y previously requested changes Aug 5, 2023
Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

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

I don't like how you replace the con command callback and then call the original. The original is just a few lines of code so I'd prefer we hook it and reconstruct it.

I can do this if you don't have the time to do so.

@catornot
Copy link
Member Author

catornot commented Aug 6, 2023

I don't like how you replace the con command callback and then call the original. The original is just a few lines of code so I'd prefer we hook it and reconstruct it.

I can't find the function for it in ghidra 😞

@F1F7Y
Copy link
Member

F1F7Y commented Aug 7, 2023

I can't find the function for it in ghidra 😞

engine.dll + 0x15B340

image

@catornot
Copy link
Member Author

catornot commented Aug 9, 2023

ghidra decided to combine Host_map_f and FUN_0015b220 lol

@catornot catornot requested review from F1F7Y and Alystrasz August 9, 2023 05:39
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Still works as expected, game does not crash when command is given an incorrect map name.

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.

Changes that I requested were addressed/are now outdated. Code looks good to me

@ASpoonPlaysGames ASpoonPlaysGames added READY TO MERGE This mergeable right now and removed needs testing Changes from the PR still need to be tested labels Sep 2, 2023
Copy link
Contributor

@Jan200101 Jan200101 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

extra check if the map actually exists may be overkill, but the vanilla map command is fragile so its probably best to guard it from inputs it may not be capable of handling

@ASpoonPlaysGames
Copy link
Contributor

extra check if the map actually exists may be overkill, but the vanilla map command is fragile so its probably best to guard it from inputs it may not be capable of handling

Game crashes if you try and do a map command with an invalid map

@Jan200101
Copy link
Contributor

yeah my tired brain thought about the wrong thing.
I ment to say no map being specified at all and just running the original command if the inputs are good.
But the command is fragile so its best to gut it completely

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

I tested the following inputs for the PR

map           <-- shows appropriate warning as expected
map mp_glich  <-- shows appropriate warning as expected
map mp_glitch <-- loads map as expected

without this PR what happens

map           <-- nothing happens
map mp_glich  <-- crashes game with error message about missing map
map mp_glitch <-- loads map as expected

Didn't do any code review as others have already covered that part anyway and I don't feel qualified enough apart from spotting basic errors.

@GeckoEidechse GeckoEidechse dismissed F1F7Y’s stale review October 2, 2023 21:15

requested changes were implemented

@GeckoEidechse
Copy link
Member

@F1F7Y I dismissed your review that requests changes as the requested changes have since been implemented as far as I can tell and the lack of a re-review was blocking the PR ^^

@GeckoEidechse GeckoEidechse merged commit cde626b into R2Northstar:main Oct 2, 2023
2 checks passed
@catornot catornot deleted the map_safeguard branch October 2, 2023 22:37
@snake-biscuits
Copy link

Has this been tested on maps which are in an installed mod but that mod is disabled?
Because those maps are listed when you type “map mp_” in console

Looking at the code changes it seems not…
Might test for myself & bug report / try a fix later

@GeckoEidechse
Copy link
Member

This breaks docker image as per pg9182/northstar-dedicated#70

GeckoEidechse added a commit that referenced this pull request Oct 3, 2023
Revert "Add a safeguard to map command (#529)"

This reverts commit cde626b.
GeckoEidechse pushed a commit that referenced this pull request Oct 3, 2023
Adds safeguard to the `map` command that prevents it from executing if the requested map is invalid or no map argument is given.
GeckoEidechse added a commit that referenced this pull request Nov 27, 2023
Adds safeguard to the `map` command that prevents it from executing if the requested map is invalid or no map argument is given.

Retry of #529

Co-authored-by: cat_or_not <[email protected]>
GeckoEidechse added a commit that referenced this pull request Nov 30, 2023
Adds safeguard to the `map` command that prevents it from executing if the requested map is invalid or no map argument is given.

Retry of #529

Co-authored-by: cat_or_not <[email protected]>
(cherry picked from commit da7061a)
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.

9 participants