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

LTS Rebalance v1.0.9 (New Verification) #16

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

Dinorush
Copy link
Contributor

@Dinorush Dinorush commented Jan 10, 2024

LTS Rebalance v1.0.9

Did I read the whole README file and understood all of its content?

YES

Did I update verified-mods.json with my new mod's information?

YES

Has a previous version of my mod been previously verified?

NO

Mod description

"Rebalances titans for better meta diversity, especially in LTS." Also of note is that it currently has a small client-side mod packaged alongside Rebalance that reparses weapon KeyValues when Rebalance is enabled/disabled at the main menu (since Northstar does not do this currently). I don't imagine it is an issue, but I figured I should mention it just in case someone is wondering what KVFix does.

Thunderstore link

https://northstar.thunderstore.io/package/Dinorush/LTS_Rebalance/

@Alystrasz
Copy link
Contributor

@Dinorush automatic verification failed because your v1.0.7 release is tagged as v1.0.7, while it should be tagged as 1.0.7 (with no leading v).
Fixing this should solve the issue :)

@ASpoonPlaysGames
Copy link

Does LTS rebalance still have the issue of needing a full game restart to work properly?

@Dinorush Dinorush changed the title LTS Rebalance v1.0.7 (New Verification) LTS Rebalance v1.0.8 (New Verification) Jan 10, 2024
@Dinorush
Copy link
Contributor Author

Dinorush commented Jan 10, 2024

To your point Spoon, that is technically not the case. It requires weapon_reparse to run before connecting to a server with different weapon key values. KVFix ensures everything is OK if you enable/disable it on the main menu. Connecting to a server that turns it off or on then does not work, though. Any client required mod which changes weapon key values has had both these issues for a while in base Northstar, actually, as the reparse that gets called never goes through. I don't know if that issue ever got fixed. I've fixed the one end I can with KVFix, the other requires a PR to Northstar. I believe Bob found the same bandaid fix I use in KVFix to get reload mods to reparse the weapons, but iirc he wanted to remake the command entirely (and never finished it). May be worth investigating any fix we can get, as key value desyncs will affect any mod adds weapon attachments or modifies values which sync with the server.

@Dinorush
Copy link
Contributor Author

From the looks of it, the python script failed because the "v" tags are prioritized so the new tag got pushed out of the initial fetched batch.

Schema failed cuz capital letters, that one's an easy fix, the other I'll look into

@Alystrasz
Copy link
Contributor

Script could not find the 1.0.8 tag in the tags list (https://api.github.com/repos/Dinorush/LTSRebalance/tags).
I feel like I have a paging issue here...

@Dinorush
Copy link
Contributor Author

Dinorush commented Jan 10, 2024

Well I renamed all my tags to remove the leading v, and consequently nuked the relationship between my releases and their tags, so rip public git release history. At least I can still scroll through the release drafts myself lol

For your own reference, looks like you can add on a page=X parameter to the API call, e.g. https://api.github.com/repos/Dinorush/LTSRebalance/tags?page=1, https://api.github.com/repos/Dinorush/LTSRebalance/tags?page=2, etc.

@Dinorush Dinorush changed the title LTS Rebalance v1.0.8 (New Verification) LTS Rebalance v1.0.9 (New Verification) Jan 10, 2024
@Alystrasz
Copy link
Contributor

I'm sorry you nuked your release history 😄
At least all lights are green for your PR to be reviewed now.

About that tags pagination, I would like to avoid fetching pages one by one, but I can't find a "sort by newest" option, so I'll probably have to do that in the end

@Alystrasz Alystrasz self-requested a review January 10, 2024 14:11
@Alystrasz Alystrasz mentioned this pull request Jan 10, 2024
@GeckoEidechse GeckoEidechse added the verification request Request for verification label Jan 10, 2024
@L1ghtman2k
Copy link

L1ghtman2k commented Jan 11, 2024

A game restart might(similar to #13) be temporarily needed here until kv issue is addressed?

@Dinorush
Copy link
Contributor Author

KVFix will at least fix it for Rebal if you reload the multiplayer lobby, so it doesn't quite need a full restart, but any normal mod that requires reparsing to prevent desync will.

@Alystrasz
Copy link
Contributor

A simple way to check if it's working is by trying:

.\NorthstarLauncher.exe -customverifiedurl=https://raw.githubusercontent.com/Dinorush/VerifiedMods/LTSRebalance/verified-mods.json

I could check that mod was correctly downloaded, but not if a game restart was needed, since I couldn't find a public server.

@Alystrasz
Copy link
Contributor

A simple way to check if it's working is by trying:

.\NorthstarLauncher.exe -customverifiedurl=https://raw.githubusercontent.com/Dinorush/VerifiedMods/LTSRebalance/verified-mods.json

I could check that mod was correctly downloaded, but not if a game restart was needed, since I couldn't find a public server.

@Dinorush could you try the above and confirm it does work?

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.

Looking at the Squirrel files, I didn't spot obvious malicious behaviour. There's no use of "dangerous" 1 functions.

As usual, verification does not ensure correctness, only non-malicious behaviour. Correctness is responsibility of the mod author.

New mod entry was tested by spawning a server requiring both LTS mods, then starting northstar client with modified manifesto as argument:

.\NorthstarLauncher.exe -customverifiedurl=https://raw.githubusercontent.com/Dinorush/VerifiedMods/LTSRebalance/verified-mods.json

Then, I could access the server through mod auto-downloading.

Footnotes

  1. e.g.: Safe I/O stuff and SquirrelHTTP. "dangerous" is a bit too much maybe. Under the assumption that the native implementations are correct and secure, there's nothing dangerous here. However it's always recommended to use more of a Swiss cheese approach and also operate a bit under the assumption that a layer can fail.

@Alystrasz Alystrasz merged commit 35aff0f into R2Northstar:master Jan 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verification request Request for verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants