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.

Sign up for GitHub

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

Jump to bottom

Cherry pick logging refactor from primedev #617

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Cherry pick logging refactor from primedev #617

wants to merge 34 commits into from

Conversation

ASpoonPlaysGames
Copy link
Contributor

Honestly I only half like this refactor, I would prefer if we used the {} style of formatting stuff among other things.

Anyway this PR isn't about my opinion so whatever

F1F7Y and others added 10 commits December 18, 2023 17:27
@F1F7Y @ASpoonPlaysGames
logging refactor (#3)
76b5049 
* Add basic logging functions

* Move to new logging functions

* Fix compilation

* Gracefully terminate when our directory isnt writable

* Add `-wconsole` argument

* Use new functions in more places

* Add helper funcs

* Thanks spoon

* Print to game console

* Stop using NS::log and spdlog:: in squirrel

* add log context string to each print

* Print to win terminal

* Don't automatically add eol

* Properly shutdown spdlog

* Fixup some messages

* Formatting

* Use spaces in CMakeLists.txt

* Formatting

* Use new funcs fol plugin log

* Always allocate console when we'rea  dedi

* Update dedilogtoclient

* Remove old logging classes

* Fix PLUGIN_LOG

* Color logging

* Don't log searchpath

* Fix masterserver error log

* Properly break in switch in spew func

* Remove comment

* Log to disk

* Format

* Terminate on fatal error call

* Improve doc comments in `dbg.cpp`

* Use displayName for plugin logging

* Improve more doc comments

* add `-nologfiles` argument

* Rename `g_bSpdLog_UseAnsiColor` to `g_bConsole_UseAnsiColor`

* Add plugin color

* Adjust colors

* Add missing newlines

* Fix specifier

* Separate SQ_GetLogContext into native and script versions

* Replace spdlog::info i missed

* Format

* Only close console once gamewindow has been created

* Improve log categories

* Fix compile error context

* Fix missed format in `scriptdatatables.cpp`

* Rename `pszAnsiString` to `svAnsiString`

* Add lock guard

* Don't try to show messagebox when we're a dedicated server on error

* Rename `fLogger` to `pLogger`

* Use new funcs for system info

* Rename `StartupLog` to `Sys_PrintOSVer`

* Initilaze `g_svLogDirectory` directly in `InitilazeNorthstar`

* Cleanup `InitilaseNorthstar`

* Add back ntdll check
@ASpoonPlaysGames
convert newer logs to refactored logging
d524bf2
@ASpoonPlaysGames
convert newer logs to refactored logging
88b5ee6
@ASpoonPlaysGames
Merge branch 'primedev-refactor/logging' of https://github.com/ASpoon…
f05a8a4 
…PlaysGames/NorthstarLauncher into primedev-refactor/logging
@ASpoonPlaysGames
merge main
4d512b2
@ASpoonPlaysGames
convert new logs
8bf0a1b
@ASpoonPlaysGames
formatting
84108ce
@ASpoonPlaysGames
fix compile
c89362b
@ASpoonPlaysGames
fix compile
06c8684
@ASpoonPlaysGames
formatting again
22ac194
@RoyalBlue1
Copy link
Contributor

cant the {} formatting be archived with std::make_format_args and std::vformat we are on cpp 20 anyways

@ASpoonPlaysGames
Copy link
Contributor Author

cant the {} formatting be archived with std::make_format_args and std::vformat we are on cpp 20 anyways

Yeah, this was just me porting the refactor over without any changes

@F1F7Y
Copy link
Member

F1F7Y commented Dec 18, 2023

cant the {} formatting be archived with std::make_format_args and std::vformat we are on cpp 20 anyways

The {} formatting comes from fmtlib which spdlog uses so you can very easily port to it if you want.

@F1F7Y
Copy link
Member

F1F7Y commented Dec 18, 2023

If i was working on refactoring logging now I'd probably have Msg, DevMsg, Warning, Error, FatalError functions where DevMsg would be stubbed in release.

Also having logging levels behind a general convar is a good idea.

@ASpoonPlaysGames
Copy link
Contributor Author

If i was working on refactoring logging now I'd probably have Msg, DevMsg, Warning, Error, FatalError functions where DevMsg would be stubbed in release.

I definitely agree about the separation between Error and FatalError, because having to pass NO_ERROR to every error log to not have the program terminate is annoying

@ASpoonPlaysGames ASpoonPlaysGames 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 Dec 18, 2023
@GeckoEidechse
Copy link
Member

I would prefer if we used the {} style of formatting stuff among other things

100% agree on that ^^"

@GeckoEidechse
Copy link
Member

So how do we approach this? Change %s back to {} before merge or merge first and then in a second PR revert back to {}?

@ASpoonPlaysGames
Copy link
Contributor Author

So how do we approach this? Change %s back to {} before merge or merge first and then in a second PR revert back to {}?

Id say changing it back would be beyond the scope of the cherry pick.

Also changing it back now would make future primedev integrations more annoying

@GeckoEidechse
Copy link
Member

I'd so, stick with %s and then change it back later once we got primedev updated?
I'm fine with whatever gets us further faster, even if that means having a temporary "regression" 1

Could test late tonight. If someone else could give it a try earlier than that, would be perfect so that we can get it merged ASAP. ^^

Footnotes

  1. Obvious remark that this is not a "regression" in the sense of that it breaks anything but it in the sense that it's style/behaviour that we might not necessary be satisfied with. ↩

@ASpoonPlaysGames
Copy link
Contributor Author

I'd so, stick with %s and then change it back later once we got primedev updated? I'm fine with whatever gets us further faster, even if that means having a temporary "regression" 1

Yup works for me, Fifty might even make some more logging refactors between now and then anyway on primedev given the conversation thus far

@ASpoonPlaysGames
Copy link
Contributor Author

Man i wish i could compile locally right now instead of having to do everything inside github

@ASpoonPlaysGames
Copy link
Contributor Author

I have moved this back to the old 1 file format, slight change to the actual text formatting though i think? idk.

anyway yeah hopefully this can get merged now as a simple code refactor

@ASpoonPlaysGames
Copy link
Contributor Author

nslog2024-01-18 21-48-31.txt
example log file as of now

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Jan 18, 2024 •
edited
Loading

Console window closing when game is launched is intentional, right?

@ASpoonPlaysGames
Copy link
Contributor Author

Console window closing when game is launched is intentional, right?

think so, yeah

@ASpoonPlaysGames
Copy link
Contributor Author

-wconsole should stop the console from disappearing

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

Merge conflicts atm btw ._.

@ASpoonPlaysGames ASpoonPlaysGames removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Jan 27, 2024
@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Jan 30, 2024
@GeckoEidechse
Copy link
Member

Looks like either #615 or #626 put merge conflicts on this PR again :c

@ASpoonPlaysGames
Copy link
Contributor Author

Should be dealt with

@github-actions github-actions bot removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Feb 8, 2024
@GeckoEidechse GeckoEidechse mentioned this pull request Feb 11, 2024
Open
@GeckoEidechse
Copy link
Member

GeckoEidechse commented Feb 11, 2024 •
edited
Loading

Testing checklist:

(If anyone already did one of these, LMK so I can check them off as done)

@Jan200101
Copy link
Contributor 

* [ ]  General Linux compat (console window close might be an issue)

Tested under Fedora 39 with and without gamescope with no issues.
Since the Steam Deck is just a modified Arch install with gamescope, there will likely be no problems.

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Feb 13, 2024 •
edited
Loading

Tested under Fedora 39 with and without gamescope with no issues.
Since the Steam Deck is just a modified Arch install with gamescope, there will likely be no problems.

Perfect, thanks <3

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Mar 4, 2024
@ASpoonPlaysGames ASpoonPlaysGames added the primedev This PR comes from cherry-picking primedev label Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
merge conflicts Blocked by merge conflicts, waiting on the author to resolve needs code review Changes from PR still need to be reviewed in code needs testing Changes from the PR still need to be tested primedev This PR comes from cherry-picking primedev
Projects
Northstar Roadmap
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ASpoonPlaysGames @RoyalBlue1 @F1F7Y @GeckoEidechse @itscynxx @Jan200101