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

Move fontfiletable.txt to keyvalues #612

Merged
merged 6 commits into from
Jul 28, 2024

Conversation

EladNLG
Copy link
Member

@EladNLG EladNLG commented Apr 10, 2023

This moves Northstar.Client/mod/resource/fontfiletable.txt to Northstar.Client/keyvalues/resource/fontfiletable.txt.

This is done so mods using keyvalues folders to add custom fonts to the game (Roguelike, HUDRevamp v2) don't revert the changes done by Northstar (though, this should be fixed)

@GeckoEidechse GeckoEidechse 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 Apr 10, 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.

Works in testing

@EladNLG
Copy link
Member Author

EladNLG commented Apr 17, 2023

Just realized uh

This is useless :3

Because launchers don't remove the previous version's files, the file will remain in the mods folder forever :))))

So the issue will forever persist.

Copy link
Member

@pg9182 pg9182 left a comment

Choose a reason for hiding this comment

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

Works fine in the dedi docker image.

@EladNLG
Copy link
Member Author

EladNLG commented Apr 18, 2023

Gecko really said "code review needed"

bro what code 😭

@F1F7Y F1F7Y added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge 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 Apr 20, 2023
@F1F7Y
Copy link
Member

F1F7Y commented Apr 20, 2023

Will test tomorrow again as i dont think i did test properly last time

@F1F7Y
Copy link
Member

F1F7Y commented Apr 20, 2023

Would like to note this has the same problem as the Mod Settings ms_slider.nut/ns_slider.nut. Considering we're going to keep running into these situations imo it'd be bseto to annoy mod manager devs to properly clean up mods when updating.

@GeckoEidechse
Copy link
Member

Would like to note this has the same problem as the Mod Settings ms_slider.nut/ns_slider.nut. Considering we're going to keep running into these situations imo it'd be bseto to annoy mod manager devs to properly clean up mods when updating.

cc @BigSpice @0neGal

Though even then, mod manager usage is around 50% of playerbase, the other half appears to still be manual install which generally are told to just overwrite files. Not sure what the best approach here is then..

@0neGal
Copy link
Contributor

0neGal commented Apr 20, 2023

Would like to note this has the same problem as the Mod Settings ms_slider.nut/ns_slider.nut. Considering we're going to keep running into these situations imo it'd be bseto to annoy mod manager devs to properly clean up mods when updating.

cc @BigSpice @0neGal

Though even then, mod manager usage is around 50% of playerbase, the other half appears to still be manual install which generally are told to just overwrite files. Not sure what the best approach here is then..

Honestly I have been meaning to look into comparing file trees and deleting files that are no longer in the newer version or similar. Or simply removing the mod folder all together.

Though, I have no clue what the best solution would be, since manual installs will also have this problem.

Another problem would be config files being reset with both of these solutions, albeit, this is a problem that's to some extent present in the current way mods are installed/updated, only difference is, on the current way if the config file doesn't come with the mod but are created by the user, they won't be reset.

Finally, another solution is just having Northstar.* mods listed as mods which should have their file tree reset with one of the methods listed above, however mod developers would then still have to deal with this problem.

I would of course be open to implementing a fix to this in Viper, if somebody finds a good solution.

@GeckoEidechse
Copy link
Member

Another problem would be config files being reset with both of these solutions, albeit, this is a problem that's to some extent present in the current way mods are installed/updated, only difference is, on the current way if the config file doesn't come with the mod but are created by the user, they won't be reset.

Yeah config files are a whole separate issue we really need to deal with especially as we are considering switching API URL in the near future. Currently tracking in R2Northstar/Northstar#430

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.

Changes look good, works in game with the file being in both mod and keyvalues directories and only in keyvalues

@EladNLG
Copy link
Member Author

EladNLG commented Jul 22, 2023

can I fucking close this

@ASpoonPlaysGames
Copy link
Contributor

If you want, with the popularity of mod managers this could honestly get merged, since a good mod manager will delete the mod before it installs the new version on update.

pinging @GeckoEidechse for visibility (although i don't expect anything tonight)

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Jul 22, 2023

can I fucking close this

Nah, please just keep the PR open. Still on my TODO list for FlightCore. Having one extra branch doesn't hurt anyone :P FlightCore supports deleting old files on update now :D

@GeckoEidechse GeckoEidechse self-assigned this Feb 24, 2024
@EladNLG
Copy link
Member Author

EladNLG commented Feb 24, 2024

HOLY SHIT!

Oh, nevermind, he just updated the branch... :[

@EladNLG
Copy link
Member Author

EladNLG commented Jul 27, 2024

Ok come ooooon why cant this be merged :[

@GeckoEidechse
Copy link
Member

Ok come ooooon why cant this be merged :[

Easy answer, it can be, was just overlooked :,)
Thanks for poking me <3

@GeckoEidechse GeckoEidechse removed the almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge label Jul 28, 2024
@GeckoEidechse GeckoEidechse merged commit 7abae6c into R2Northstar:main Jul 28, 2024
3 checks passed
@GeckoEidechse GeckoEidechse removed their assignment Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants