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

Disable MAD on vanilla #636

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

itscynxx
Copy link
Contributor

Simply adds a check for the ON_DLL_LOAD that runs the mod auto download code for if vanilla compatibility is enabled.

Although it would never run on vanilla, some people are confused by the Custom verified mods URL not found in command line arguments, using default URL. and Mod downloader initialized added in logs when loading mod auto download, and, from testing, doesn't break anything to disable in this way. I was able to play a match of vanilla and a match of northstar without issues

No idea if this can be done better, just looked at what Spoon did for the master server auth and put it here

Can't test right now, on my laptop, might need to make a pr to my fork or the actual repo to test it because my local files seem to get 800+ errors when trying to build...
@itscynxx
Copy link
Contributor Author

Could someone explaining why the format check fails?

as far as I'm aware I do the same thing as done here: https://github.com/R2Northstar/NorthstarLauncher/blob/main/primedev/masterserver/masterserver.cpp#L605

unless this is some tabs vs spaces thing ig

@ASpoonPlaysGames
Copy link
Contributor

Could someone explaining why the format check fails?

You have some trailing whitespace in an empty line

@ASpoonPlaysGames
Copy link
Contributor

ASpoonPlaysGames commented Jan 12, 2024

Could someone explaining why the format check fails?

You have some trailing whitespace in an empty line

Or at least that's what format check is saying
image

@itscynxx
Copy link
Contributor Author

Could someone explaining why the format check fails?

You have some trailing whitespace in an empty line

That did it, thanks

@Alystrasz
Copy link
Contributor

What's the exact procedure to launch vanilla through Northstar and have access to logs?

@ASpoonPlaysGames
Copy link
Contributor

What's the exact procedure to launch vanilla through Northstar and have access to logs?

launch northstar with the -vanilla launch arg

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.

PR confirmed working in testing, g_pModDownloader isn't initialized on vanilla plus, effectively removing related messages.

Without PR
[2024-01-14] [00:08:31] [NORTHSTAR] [info] Enabling hook Status_ConMsg
[2024-01-14] [00:08:31] [NORTHSTAR] [info] Enabling hook CClientState_ProcessPrint
[2024-01-14] [00:08:31] [NORTHSTAR] [info] Registering Convar spewlog_enable
[2024-01-14] [00:08:31] [NORTHSTAR] [info] Mod downloader initialized
[2024-01-14] [00:08:31] [NORTHSTAR] [info] Custom verified mods URL not found in command line arguments, using default URL.
[2024-01-14] [00:08:31] [NORTHSTAR] [info] Enabling hook OpenExternalWebBrowser
[2024-01-14] [00:08:31] [NORTHSTAR] [info] Registering Convar ns_prefer_datatable_from_disk
With PR
[2024-01-14] [00:12:52] [NORTHSTAR] [info] Enabling hook Status_ConMsg
[2024-01-14] [00:12:52] [NORTHSTAR] [info] Enabling hook CClientState_ProcessPrint
[2024-01-14] [00:12:52] [NORTHSTAR] [info] Registering Convar spewlog_enable
[2024-01-14] [00:12:52] [NORTHSTAR] [info] Enabling hook OpenExternalWebBrowser
[2024-01-14] [00:12:52] [NORTHSTAR] [info] Registering Convar ns_prefer_datatable_from_disk

@GeckoEidechse GeckoEidechse added the needs code review Changes from PR still need to be reviewed in code label Jan 13, 2024
@GeckoEidechse
Copy link
Member

I'm kinda curious what would happen if MAD were to be called in vanilla (e.g. via some mod). Just null pointer deref and crash ig?

@itscynxx
Copy link
Contributor Author

I'm kinda curious what would happen if MAD were to be called in vanilla (e.g. via some mod). Just null pointer deref and crash ig?

Me too, got any easy way of testing it?
Not sure if just calling DownloadMod does all of it or not and it's kinda late to be thinking here lol

@ASpoonPlaysGames
Copy link
Contributor

I'm kinda curious what would happen if MAD were to be called in vanilla (e.g. via some mod). Just null pointer deref and crash ig?

The various SQ functions should have checks added to ensure that they don't do this.

@catornot
Copy link
Member

The various SQ functions should have checks added to ensure that they don't do this.

It would be better if they wouldn't even get registered at all if MAD is disabled.

@ASpoonPlaysGames
Copy link
Contributor

The various SQ functions should have checks added to ensure that they don't do this.

It would be better if they wouldn't even get registered at all if MAD is disabled.

True, but that's more complex, and ADD_SQFUNC doesn't support it

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Jan 20, 2024
@github-actions github-actions bot removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Jan 20, 2024
@Alystrasz
Copy link
Contributor

Me too, got any easy way of testing it? Not sure if just calling DownloadMod does all of it or not and it's kinda late to be thinking here lol

I believe calling DownloadMod would straight up crash the client, which is better than downloading anything I guess

@GeckoEidechse
Copy link
Member

With #751 merged, is this still relevant?

@ASpoonPlaysGames
Copy link
Contributor

ASpoonPlaysGames commented Aug 15, 2024

The error message is probably gone now, but if we are in vanilla mode it's probably better to just never instantiate the mod downloader?

Would be worth checking the codebase for things that grab the static instance and make sure that they check for vanilla compatibility first as well

@GeckoEidechse
Copy link
Member

Yeah, if we avoid loading MAD on vanilla then we also need to check that the uninstantiated object is never loaded cause otherwise it's gonna crash on reading random uninitialised memory.

@ASpoonPlaysGames
Copy link
Contributor

Yeah, if we avoid loading MAD on vanilla then we also need to check that the uninstantiated object is never loaded cause otherwise it's gonna crash on reading random uninitialised memory.

This is why just grabbing static things blindly is bad

@GeckoEidechse
Copy link
Member

Putting into draft so that I know what to focus on and as per discussion this PR is blocked by the lack of checks that we do not access the uninitialised object. @itscynxx if you (or anyone else) disagrees, feel free to undraft it as is or leave a comment for me to undraft and we can take another look.

@GeckoEidechse GeckoEidechse marked this pull request as draft August 19, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs code review Changes from PR still need to be reviewed in code
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants