Use spdlog for logging

[filipwasil]

https://github.com/gabime/spdlog

This library is great for logging. It may be a bit difficult but we really should consider using it instead of old fashioned FLOG_XXX macros. Sometimes they are so lame to work with...

1) add this library as git submodule in ext directory 2) Provide each module with appropriate headers 3) Replace all usages of FLOG's with spdlog

[filipwasil]

Recently I added a Logger class.

This logger class is what needs to be replaced. Together with all stuff under Log.h

[KeramKeram]

I will try help on this in second part of the week. I can't promise miracles :)

[filipwasil]

Perfect !

[KeramKeram]

spdlog is add to ext folder and external.cmake. I'm adding spdlog to cmakes in cmake/platform. First linux. Should non-dev(linux.cmake) have logs? Now I have small problem with shadow declaration, I must delete old log system.

[filipwasil]

dev targets are meant to be used only for headers installation purpose. There is no actual library built there. Just headers to be installed.

On ubuntu spdlog can be installed using libspdlog-dev. Probably similar name for fedora.

I think that we can use spdlog it in exactly the same way we use glm. Note the Math.h file. In this case we will have Log.h with spdlog headers.

[filipwasil]

I do not know if you noticed that you can use zube of github to make notes. They will sync in both directions (Which is pretty cool !).

[KeramKeram]

Ah ok, I was think that spdlog are simply headers(like they say on the site) we take them.

Yes fedora have spdlog.

I think I will end this task on the weekend.

[KeramKeram]

Ok, I'm change from FLOG to console->XXX(in src folder). I was busy during weekend and I will end this during the week. One more question, ok I can use provided by linux libraries but like glm I must add spdlog from ext to android,ios ckames etc. In short, to all .cmake files where I can find glm(I want use spdlog like glm).

[filipwasil]

Perfect !

niedz., 9.10.2016, 22:15 użytkownik revcorey notifications@github.com napisał:

Ok, I'm change from FLOG to console->XXX. I was busy during weekend and I will end this during the week. One more question, ok I can use provided by linux libraries but like glm I must add spdlog from ext to android,ios ckames etc. In short, to all .cmake files where I can find glm(I want use spdlog like glm).

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-252509685, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvdnx9APPsP5weTPXloRRnJAHaeesks5qyUtDgaJpZM4KDOXj .

[KeramKeram]

Hey, Two questions before I will change this: -I will include spdlog in Log.h and remove all old FLOG(or move them to deprecated namespace) -I will create small function who return std::shared_ptr inside I will try to get existing console but if not exist create new one(name "LogConsole"). Are you ok with that?

[filipwasil]

Upload a review :+1:

2016-10-09 22:42 GMT+02:00 revcorey notifications@github.com:

Hey, Two questions before I will change this: -I will include spdlog in Log.h and remove all old FLOG(or move them to deprecated namespace) -I will create small function who return std::shared_ptr inside I will try to get existing console but if not exist create new one(name "LogConsole"). Are you ok with that?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-252511320, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvYx4pRZ73Z5tX4ePKEvaNanrTU6Vks5qyVG1gaJpZM4KDOXj .

Pozdrawiam serdecznie / Best Regards

Filip Wasil

filipwasil.bitbucket.org

[filipwasil]

Do not move things to deprecated namespaces. Just remove them :)

[KeramKeram]

I added first draft, It need formating chanegs, cmake changes etc. But most FLOG_XXX move to spd log but there is one small trick:

• You can still use for example FLOG_ERROR("bla %d",XYZ); -If log have only message FLOG_ERROR("bla") you must add NULL as argument FLOG_ERROR("bla", NULL) I know it is tricky but you accept this? What to do with android logs? They are use some android macro.

[KeramKeram]

I will try this .Null argument fix.

[filipwasil]

Maybe we should try to use different naming for logging functions ? What do You think ? Instead of FLOG_ERROR style we can use:

• floge, flogd, flogf
• fillwave::loge, fillwave::logd, fillwave::logf
• logfe, logfd, logff

i think all of them are better than FLOG_ERROR

[KeramKeram]

Hmm FLOG_XXX is better to understand if you see this code first time but I think we can use:

• floge, flogd, flogf

I will commit changes this night or tomorrow morning. As for changes in .cmake files I will reassigned them to Marta(I do some changes in cmake but not yet end them.). It will be star point for her.

[filipwasil]

Maybe try FLOG_E. FLOG_D, FLOG_F

shorter, but stilll valid as a macro name.

[KeramKeram]

This class fillwave::Logger was only use for logging? I'm currently removing this entire class, I think we do not need them.

"Maybe try FLOG_E. FLOG_D, FLOG_F

shorter, but stilll valid as a macro name." Unfortunately I was force to use static function instead of macros. But names changes.

[filipwasil]

Confirmed. Approved to remove. if this is not a macro then flogf, flogd seems better to me.

[filipwasil]

Maybe using the camel case:

fLogE fLogD

?

[filipwasil]

Yep using the camel case here will make the name valid and well readable.

fLogE fLogD fLogU fLogF fLogI

please :)

[KeramKeram]

On gerrit. Fillip can you check ios/android build? I reset my previous changes in cmakes(spdlog in externals) I will reassigned marta to this. I will explain her everything tommorow. She must:

• Add spdlog like glm
• Update README file.

[filipwasil]

i am checking android build right now. Reviewed.

[filipwasil]

Android /home/filip/Projects/fillwave/inc/fillwave/Log.h:39:27: fatal error: spdlog/spdlog.h: No such file or directory

[KeramKeram]

hmmm Ah, first changes in cmakes I mean add spdlog to externals(now I removed this from commit because I want this task for marta).

[KeramKeram]

Wait we should not use spdlog for android. I know now what to do simply #ifdef ?

[filipwasil]

Ok i see couple of problems with the code:

• Headers android typical are not inside compiler switches
• Creating shared pointer every time when we want to log something is killing the performance.
• creating a 1000 byte table every time when we want to log will killl the performance as well

I suggest to create one copy of logger for each module (make it static in header), same with the 1000 byte buffer. do the null check once

[KeramKeram]

Ok, I will do this tomorrow.

[filipwasil]

Basically, the fLogE should actually only call logs->critical(buffer); All the other stuff should happen only once. During some sort of initialization. There should be no shared pointer copy done (Exept initialization)

2016-10-16 22:03 GMT+02:00 revcorey notifications@github.com:

Ok, I will do this tomorrow.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-254070746, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvQGgAJ41VSxfXD1uikiWb6PmKY6Lks5q0oMigaJpZM4KDOXj .

Pozdrawiam serdecznie / Best Regards

Filip Wasil

filipwasil.bitbucket.org

[KeramKeram]

Yes I know what you have on mind :)

[filipwasil]

One more thing. glGetError() should be removed from the header and should be wrapped by other function just like in case of getFramebufferStatus

Then, the OpeGL.h header will not be needed. Cheers !

getFramebufferStatus

unsigned int getFramebufferStatus() { return glCheckFramebufferStatus(GL_FRAMEBUFFER); }

2016-10-16 22:37 GMT+02:00 revcorey notifications@github.com:

Yes I know what you have on mind :)

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-254072942, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvfJBFhL0MxNjiwa2unBOgekusXPsks5q0orzgaJpZM4KDOXj .

Pozdrawiam serdecznie / Best Regards

Filip Wasil

filipwasil.bitbucket.org

[KeramKeram]

getFramebufferStatus etc. before was in macro now it is function! I need this header!

[filipwasil]

Ok You can leave it . Logging itself is much more important :)

wt., 18.10.2016, 21:22 użytkownik revcorey notifications@github.com napisał:

getFramebufferStatus etc. before was macro now it is function! I need this header!

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-254611671, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvQ3QTvI6BZbJy5j5m7hKWTFC1P9qks5q1RxygaJpZM4KDOXj .

[KeramKeram]

Perhaps move this include to #if statement where is now spdlog function for debug? Never in relase or android use.

[filipwasil]

good idea. Go for it ! :+1:

[KeramKeram]

I will create new w review because this review do not have Change-Id when was created.

[filipwasil]

You are looking for this:

refs/changes/89/298389

2016-10-18 21:37 GMT+02:00 revcorey notifications@github.com:

I will create new w review because this review do not have Change-Id when was created.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-254615635, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvbD1B4fSKq7i1ocpfkkWa-iwaFsKks5q1R_fgaJpZM4KDOXj .

Pozdrawiam serdecznie / Best Regards

Filip Wasil

filipwasil.bitbucket.org

[filipwasil]

Just ammend your change and do:

git push HEAD:refs/changes/89/298389

You can view the id by opening the "Patch Sets" or "Download" menu in the upper right in gerrit view

[filipwasil]

Did You see the macro i used in old loggers to get the content of the variadic arguments? Is this not usable in this case ?

wt., 18.10.2016, 21:39 użytkownik Filip Wasil filip.wasil@gmail.com napisał:

You are looking for this:

refs/changes/89/298389

2016-10-18 21:37 GMT+02:00 revcorey notifications@github.com:

I will create new w review because this review do not have Change-Id when was created.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-254615635, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvbD1B4fSKq7i1ocpfkkWa-iwaFsKks5q1R_fgaJpZM4KDOXj .

Pozdrawiam serdecznie / Best Regards

Filip Wasil

filipwasil.bitbucket.org

[KeramKeram]

You meant return to macros instead of function, in the beginning I was reinitialize/initialize logger in function that was the reason to move from macro to function but now hmmm I will check.

[filipwasil]

VA_ARGS i used it to pass the arguments. I am not sure if it can be used here as well. Reviewing by phone ... Damn ! So bad ...

wt., 18.10.2016, 22:31 użytkownik revcorey notifications@github.com napisał:

You meant return to macros instead of function, in the beginning I was reinitialize/initialize logger in function that was the reason to move from macro to function but now hmmm I will check.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-254630206, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvZm97dSaENiqNQ2f_TGs9GDg1LCmks5q1SyVgaJpZM4KDOXj .

[KeramKeram]

Quick check, I done some simple test:

define fLogW(...) FLOG_BASE(GPU_WARNING, VA_ARGS)

 enum GPU_LOG_TYPE {
     WARNING
 };
#define FLOG_BASE(type, ...)                                          \
    do {                                                                            \
                vsnprintf(spdlogBuffer, 1000, __VA_ARGS__);                         \
                switch(type)                                                        \
 {                                                                                  \
     case WARNING: logs->critical(spdlogBuffer); break;                             \
     default: logs->critical(spdlogBuffer); break;                                   \
 }                                                                                  \
                                                                     \
    } while(0)

Compilation goes ok. Tommorow I will update review, and instead of standard enum i will use enum class.

[filipwasil]

Perfect !

wt., 18.10.2016, 22:52 użytkownik revcorey notifications@github.com napisał:

Quike check, I done some simple test:

define fLogW(...) FLOG_BASE(GPU_WARNING, _VAARGS)

enum GPU_LOG_TYPE { WARNING };

define FLOG_BASE(type, ...) \

do {                                                                            \
            vsnprintf(spdlogBuffer, 1000, **VA_ARGS**);                         \
            switch(type)                                                        \

{ \ case WARNING: logs->critical(spdlogBuffer); break; \ default: logs->critical(spdlogBuffer); break; \ } \ \ } while(0)

Is' compiling. Tommorow I will update review, and instead of standard enum i will use enum class.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/filipwasil/fillwave/issues/93#issuecomment-254636065, or mute the thread https://github.com/notifications/unsubscribe-auth/ABxGvfYbeg5I0Bo_wqxq5e7FhVyN7KQEks5q1TGlgaJpZM4KDOXj .

[KeramKeram]

Unfortunately when macro have more than one parameter it fails. We can only use functions, but I changed functions from static to inline, what you think?

[KeramKeram]

I uploaded new version with inline and pleas test on android and on linux with some example.

[filipwasil]

Note the commit. WHat do you think ?

When using on linux it throws an exce[tion:

terminate called after throwing an instance of 'spdlog::spdlog_ex' what(): logger with name LogConsole already exists

Another thing is that we should have a base macro for logging, and that this function:

spd::stdout_logger_mt("LogConsole")

in my case does not have second argument (probably we have different spdlog version)

[filipwasil]

Review updated. All issues fixed. Optimized for code speed and size. How do you think @revcorey ?

[KeramKeram]

Ok, I compiled examples myself and checking this, This throw is possible, work in progress.

second argument is to on coloring. It is copied from spdlog examples.

[KeramKeram]

Ok beacuse of GPU drivers I'm unable start example but before it's shutdown I can reproduce problem, I fixed this by taking shader_ptr in to function. But speed should be horrible against for example raw pointers.

[KeramKeram]

I pushed another review with shared_ptr in function can you check impact of this change? Check how much FPS we drop on examples(I'm unlabeled to to this because of current GPU driver)

https://review.gerrithub.io/#/c/299258/