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

Implement spdlog and Logging Service #82

Draft
wants to merge 15 commits into
base: master
from

Conversation

@capnkenny
Copy link
Member

capnkenny commented Oct 30, 2019

PR Resolves #12

  • Implements a spdlog based logging service, as well as moves all current stdout and stderr logging into the service functions.

  • Defines specific "core" logger names to help make logging distinct based on functionality/where it is called.

  • Utilizes an asynchronous & multi-threaded method of logging based on stdout - allows for different threads to call for logging at anytime (helpful if a particular logger gets shared - SDL2_Mixer shouldn't complain either as its run on a separate thread iirc).

It was not defined if this was to cover file logging but that should be able to be added pretty easily if needed (maybe in separate PR).

capnkenny added 9 commits Oct 24, 2019
Aligned Logging Service with requirements
Implemented Basic Logging

Logging is initialized by default, either at TRACE for debug builds or INFO for non-debug builds.

This is set with CMake's NDEBUG flag - all we need to do is set the build type with -DCMAKE_BUILD_TYPE=...
Bilbo Loggins - Chapter 1

Did what the title says, and switched NovelAudioService to find the existing logger and use it instead of std::cout.
Removed dependencies where nothing is being logged - namely NovelRunner changes previously done.

Switched to singleton instance of Logging so that everything accesses the same logger.
Removed singleton instance - spdlog already uses them, and I was just spraying the heap with more, causing corruption.

Now launches 10 out of 10 times!
Since spdlog is singleton-based, we utilize that to our advantage here, so we're not really creating a brand new logger every time.

Added constants to help separate, and added Asynchronous logging.

Also finished editing current ERROR/INFO messages to match new format.
@capnkenny capnkenny self-assigned this Oct 30, 2019
capnkenny added 4 commits Oct 30, 2019
Removed unnecessary string vars from being created when we can get the values converted during logging.
Finished removing references to removed var.
Build error on Ubuntu CI buildbot - specific to non-windows platforms only
@capnkenny capnkenny requested review from RubyNova and tannergooding and removed request for RubyNova Oct 30, 2019
Copy link
Member

RubyNova left a comment

Looks functionally fine, formatting is kind of a nightmare though.

src/NovelAudioService.h Outdated Show resolved Hide resolved
src/NovelImageRect.cpp Outdated Show resolved Hide resolved
src/NovelLoggingService.cpp Outdated Show resolved Hide resolved
std::cerr << "SPDLOG ERROR: " << msg << std::endl;
});
}
catch (const spdlog::spdlog_ex & ex)

This comment has been minimized.

Copy link
@RubyNova

RubyNova Nov 3, 2019

Member

formatting error here. Plzfix.

This comment has been minimized.

Copy link
@capnkenny

capnkenny Nov 5, 2019

Author Member

Was it the #ifndef part here?

This comment has been minimized.

Copy link
@RubyNova

RubyNova Nov 5, 2019

Member

Nope. Check your reference specifier.

src/NovelLoggingService.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
initializeAudio();
}

bool NovelAudioService::initializeAudio() {
if (SDL_InitSubSystem(SDL_INIT_AUDIO) < NovelUtilities::SDL_SUCCESS)
{
std::cout << "ERROR: Cannot play audio!" << std::endl;
std::cout << "SDL Error: " << SDL_GetError() << std::endl;
_console.log("Cannot play audio: " + std::string(SDL_GetError()) , LogLevel::ERR);

This comment has been minimized.

Copy link
@tannergooding

tannergooding Nov 3, 2019

Contributor

nit: It might be nice for us to declare a helper function like: LogIfSdlFailure(SDL_InitSubSystem(SDL_INIT_AUDIO), "Cannot play audio")

That would make the methods smaller and would isolate/make consistent the error retrieval and formatting

This comment has been minimized.

Copy link
@tannergooding

tannergooding Nov 3, 2019

Contributor

It would also allow more customization on what happens for particular failure kinds (as some of these should likely be throwing rather than just logging and continuing).

This comment has been minimized.

Copy link
@capnkenny

capnkenny Nov 5, 2019

Author Member

May need to discuss this in Discord perhaps. Not sure how to go about handling this (properly).

}
else
{
std::cout << "INFO: Mixer engaged!" << std::endl;
_console.log("SDL2_Mixer Initialized.", LogLevel::INFO);

This comment has been minimized.

Copy link
@tannergooding

tannergooding Nov 3, 2019

Contributor

nit: Maybe have helper functions like logInfo, logError, etc?

@@ -47,7 +45,7 @@ void NovelAudioService::load(std::string input, bool isMusic) {
}
else
{
std::cerr << "ERROR: " <<Mix_GetError() << std::endl;
_console.log(std::string(Mix_GetError()), LogLevel::ERR);

This comment has been minimized.

Copy link
@tannergooding

tannergooding Nov 3, 2019

Contributor

nit: Might be nice if there was an overload that automatically called std::string for the errorCode/statusCode case

@@ -103,7 +101,7 @@ void NovelAudioService::playSound(std::string soundName, int loops) {
{
if (Mix_Playing(_channelMap[soundName]) == MIXER_TRUE)
{
std::cout << "Already playing on channel " << _channelMap[soundName] << std::endl;
_console.log("Already playing on channel " + _channelMap[soundName], LogLevel::WARN);

This comment has been minimized.

Copy link
@tannergooding

tannergooding Nov 3, 2019

Contributor

nit: Would be more efficient if this took 2 separate strings and logged them sequentially (as this will allocate and create a new bigger string)

@@ -38,6 +39,7 @@ class NovelImageRect : public NovelRenderObject {
void configureObjectBuffers() final;

private:
NovelLoggingService _console;

This comment has been minimized.

Copy link
@tannergooding

tannergooding Nov 3, 2019

Contributor

Why does ImageRect need to hold onto the logging service?

This comment has been minimized.

Copy link
@capnkenny

capnkenny Nov 5, 2019

Author Member

Currently ImageRect performs file-handling for loading images - I'd gladly take this away (the hold on the logging service), but I'm not sure how we would go about referencing it to call the log in NovelImageRect.cpp since it doesn't onto a service that already holds it.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Nov 5, 2019

Contributor

There should likely be a way to get the logging service (or services in general) without holding onto them (you don't want images/meshes/etc each holding onto the service)...

cc. @RubyNova. Thoughts here?

I would think for that to happen, there would need to be some static method for getting the current "application" (NovelRunner) and therefore being able to get everything else...

capnkenny added 2 commits Nov 5, 2019
Fixed most (if not all) of issues addressed in RubyNova's review. Also caught additional CMakeLists issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.