Send all mods to Atlas that are enabled

[ASpoonPlaysGames]

Instead of just RequiredOnClient mods and mods that have pdiff (lol)

Just a QOL thing really

Merge together with R2Northstar/NorthstarDocs#128

[GeckoEidechse]

One thing to consider is whether that might expose mods on the server that server hosters don't wanna have exposed, e.g. some custom anti-cheat, Spyglass (maybe?), etc...

[ASpoonPlaysGames]

One thing to consider is whether that might expose mods on the server that server hosters don't wanna have exposed, e.g. some custom anti-cheat, Spyglass (maybe?), etc...

Spyglass is absolutely public, but i see the point. Perhaps having an explicit "dont send that I have this mod" field in mod.json is best?

[EnderBoy9217]

Yeah I would recommend making it a separate mod.json field that sends by default (for existing mod compatibility)

[pg9182]

expose mods on the server that server hosters don't wanna have exposed

If this is merged, I'll make atlas only send client-required mods in the server list.

[EnderBoy9217]

Doesn’t it already do that?

[ASpoonPlaysGames]

expose mods on the server that server hosters don't wanna have exposed

If this is merged, I'll make atlas only send client-required mods in the server list.

Wait but that kinda defeats the point of the PR though, no?

I mean it'll still be nice for metrics, but a mod name + version really isnt an issue imo. If a server really wants to obfuscate a mod, just change it's name or something idk

[GeckoEidechse]

If a server really wants to obfuscate a mod, just change it's name or something idk

Then we should ping server hosters before release to make them aware IMO.

[F1F7Y]

The entire point of this is so clients can know what mods the server uses.
Making atlas only send the ones required on client is just a step back and completely invalidates this PR. The same goes for server hosters being able to choose if a server is broadcasted.

[Jan200101]

Code is as simple as it gets.
Played 2 rounds of Titan Brawl with 0 issues

[F1F7Y]

FYI required on client mods aren't exposed on script so script treats all mods as required on client.

[ASpoonPlaysGames]

FYI required on client mods aren't exposed on script so script treats all mods as required on client.

Yes they are? https://github.com/R2Northstar/NorthstarMods/blob/main/Northstar.Client/mod/scripts/vscripts/ui/menu_ns_modmenu.nut#L198

[F1F7Y]

Yes they are? https://github.com/R2Northstar/NorthstarMods/blob/main/Northstar.Client/mod/scripts/vscripts/ui/menu_ns_modmenu.nut#L198

Script uses this to compare local and remote mods. This struct had no requiredonclient field meaning clients will need to match mods and their versions to be able to connect
https://github.com/R2Northstar/NorthstarMods/blob/main/Northstar.Client/mod/scripts/vscripts/cl_northstar_client_init.nut#L24-L28

[ASpoonPlaysGames]

Script uses this to compare local and remote mods. This struct had no requiredonclient field meaning clients will need to match mods and their versions to be able to connect https://github.com/R2Northstar/NorthstarMods/blob/main/Northstar.Client/mod/scripts/vscripts/cl_northstar_client_init.nut#L24-L28

Ah, yeah fair enough. Ig that blocks this PR until a corresponding mods PR is made or something

[GeckoEidechse]

I'll update the label accordingly

[GeckoEidechse]

Also this PR should really be tested with a listen/dedi server and another client that does not have the exact same mods trying to join.

[ASpoonPlaysGames]

Looking at launcher code, it seems that the required mod info is properly filtered, and doesn't blindly just use everything that it gets from Atlas:

NorthstarLauncher/NorthstarDLL/masterserver/masterserver.cpp

Line 312 in db5eac1

if (!requiredMod.HasMember("RequiredOnClient") || !requiredMod["RequiredOnClient"].IsTrue())

So I don't think that this PR would actually cause any information to be wrong without a mods PR.

I could make this PR also expose non-required mods to squirrel if that's wanted? If I do that it would make this PR depend on a mods PR that edits the structs though

[GeckoEidechse]

I was asked for my opinion on this.

Basically I'm still split. For example imagine you are hosting a quick listen server for your friends but you also have some rather sus/NSFW mods installed. Do you really wanna have that information leaked and potentially scraped by others?
(For reference, I scrape the public server browser list every 10 minutes for stat collection and make it public via https://github.com/GeckoEidechse/NorthstarServerBrowserData)

So personally I'd prefer if we had some way that allows for publishing information like server-only mods like Headhunters without leaking any mods that potentially you wouldn't wanna have published.

[EnderBoy9217]

Couldn’t we have an option in mod.json that allows the mod to be hidden (which the sus mods or really all client-only mods could use), but by default have the mod shown in the server browser (like the mod doesn’t specify so it shows, backwards-compatibility and such).

[GeckoEidechse]

Couldn’t we have an option in mod.json that allows the mod to be hidden (which the sus mods or really all client-only mods could use), but by default have the mod shown in the server browser (like the mod doesn’t specify so it shows, backwards-compatibility and such).

The thing is that would require mods to be updated first which tbh is never gonna happen for large chunk of them. At the same time the same could be said about an opt-in system for server only mods. Tbh, I don't really have a good answer here...

[EnderBoy9217]

That’s why I mentioned that they would be on by default and without any required mod.json additions, that way the mods that won’t update will still show and won’t be required to update.

[ASpoonPlaysGames]

Couldn’t we have an option in mod.json that allows the mod to be hidden (which the sus mods or really all client-only mods could use), but by default have the mod shown in the server browser (like the mod doesn’t specify so it shows, backwards-compatibility and such).

This was discussed above, having it optional defeats the entire point of it. A case can be made for passworded servers not displaying it, but public servers nah

[ASpoonPlaysGames]

I could make this PR also expose non-required mods to squirrel if that's wanted? If I do that it would make this PR depend on a mods PR that edits the structs though

but also @GeckoEidechse i shouldve been more specific, i wanted your opinion on this specifically lol

[ASpoonPlaysGames]

solved conflicts by force pushing

[GeckoEidechse]

I still think we shouldn't do this as it leaks mods that clients are running when doing a listen server. The idea behind this PR is to more easily see which servers are running mods like Kraber9k but for this something like a tag systems or the like would be preferred.

[ASpoonPlaysGames]

I still think we shouldn't do this as it leaks mods that clients are running when doing a listen server. The idea behind this PR is to more easily see which servers are running mods like Kraber9k but for this something like a tag systems or the like would be preferred.

I still fail to see the issues with "leaking" mods. If you don't like others seeing the names (not necessarily even the content) of your mods then why are you even running them on your server in the first place? Even so, we could just make this only send required mods for listen servers and send all mods for dedis if we must compromise a bit on this. :/

Also, what tag system? This gets brought up fairly often but over like 2 years there has been very little traction on actually implementing anything. Why prevent a good change in the hopes of a better one in the future? It's not like this change would be difficult to revert or anything.

[EM4Volts]

I dont see the problem with sending tbh.
if thats was "send all mods to server user connects to" then hell no, but to atlas its fine imo.
maybe only send mods that are required on client and also server.
i fill it could be VERY usefull in the future for pdiff as a system overseen by atlas is potentially a thing pdiff needs idk.
i just hope this wont be used potentially in the future to ban people should we ever decide on a ban system for atlas as then people could just troll friends by calling some mod "aimbot" in the json and sending them the mod.

[EM4Volts]

One thing to consider is whether that might expose mods on the server that server hosters don't wanna have exposed, e.g. some custom anti-cheat, Spyglass (maybe?), etc...

rename it to something else then. a custom anticheat could just rename its name field in the json... besides that, even if i see a custom anti cheat why would any normal player care. if they dont cheat its not gonna hurt them

[GeckoEidechse]

If you don't like others seeing the names (not necessarily even the content) of your mods then why are you even running them on your server in the first place?

My concern is players hosting listen servers and not being informed that their mod list will be public. There's no warning or anything happening which is bad UX. If there was a warning with "Your mod list will be public" upon starting a listen server, I'd be fine with the change ^^

[ASpoonPlaysGames]

My concern is players hosting listen servers and not being informed that their mod list will be public. There's no warning or anything happening which is bad UX. If there was a warning with "Your mod list will be public" upon starting a listen server, I'd be fine with the change ^^

To host a listen server you need to port forward. I think that a warning in the docs there would be enough.

[GeckoEidechse]

To host a listen server you need to port forward. I think that a warning in the docs there would be enough.

Works for me. Feel free to PR and then we merge both at the same time o7

[Alystrasz]

Did you test this change on a server?

My fear is that currently, clients must have all server-exposed (in server.requiredMods) mods locally installed to be able to join said server; if non-RequiredOnClient mods are exposed, they won't be downloadable through MAD and thus will prevent server join.

[ASpoonPlaysGames]

My fear is that currently, clients must have all server-exposed (in server.requiredMods) mods locally installed to be able to join said server; if non-RequiredOnClient mods are exposed, they won't be downloadable through MAD and thus will prevent server join.

Feels like we should have been checking that the mod was in fact required and not just assuming that it was ngl. Any mod that added a mod.pdiff file would have caused this issue (even though that file has no functionality) with the existing code

[ASpoonPlaysGames]

On further inspection, we do sanitise this in

NorthstarLauncher/primedev/masterserver/masterserver.cpp

Line 323 in 4ff226d

if (!requiredMod.HasMember("RequiredOnClient") || !requiredMod["RequiredOnClient"].IsTrue())

This means that there will be no user-facing changes with just this PR, and an accompanying PR or two would have to be made to show this new data in the server browser.

[Alystrasz]

Code looks good to me.
I tested this by running a private match with several mods, and can confirm only RequiredOnClient mods are exposed (checking through server browser):

Server mods

Required mods

[Alystrasz]

Works for me. Feel free to PR and then we merge both at the same time o7

With R2Northstar/NorthstarDocs#128, I set this as ready to be merged.