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

Log levels and modules - v0.2 milestone #1388

Merged
merged 17 commits into from Aug 16, 2018
Merged

Log levels and modules - v0.2 milestone #1388

merged 17 commits into from Aug 16, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 13, 2018

Ready for merging!
This implements log levels and modules. If the level of the event is less than the current selected level, it won’t be printed. If the module if off, all its event won’t be printed regardless of the level. Also note the messages printed with printf will always be printed independently of level and modules. This is useful if you want to always print a message (e.g.: xbe signature check to avoid disabling its printing in the log). Other messages will only be printed on debug builds (e.g. DbgPrintf, LOG_FUNC_BEGIN) because on release builds their macro decays to a null function or it’s empty, but they will still respect log levels and modules. Finally, a new logging window has been added to the gui to let users modify the log parameters, also at runtime.

log

@ghost ghost added enhancement general improvement of the emu user interface GUI related labels Aug 13, 2018
@PatrickvL
Copy link
Member

LOG_PREFIX didn't have to be an argument if we make sure that symbol is always in scope

@RadWolfie
Copy link
Member

RadWolfie commented Aug 14, 2018

I had made a test from AppVeyor (release build), it does not seem to output any of the specific log info to console. Almost as if nothing has change. EDIT: See my second comment below.

I discover a bug for "Emulator" event list. If I enable all, close emulator, re-open, open logging dialog window. Then SMC checkbox is unchecked.

Also, accept / cancel button is a requirement in case anyone decide to not save the new change.

As for sending log configured update over to runtime emulation does not make any sense. It should be static to keep the log more complete. Plus there is an hotkey to toggle the output log.

@RadWolfie
Copy link
Member

RadWolfie commented Aug 14, 2018

Other messages will only be printed on debug builds (e.g. DbgPrintf, LOG_FUNC_BEGIN) ...

Actually, the reason LOG_FUNC_BEGIN and debug messages are on debug build is because of no log filter support. Plus we need release build to able output these as well since log filter, this pull request, is now supported. Then we can finally have our testers to send it to one of developer/contributor for specific filter to review through without intensive going through all outputs greater than 1 GB or more file size in matter of seconds.

@ghost
Copy link
Author

ghost commented Aug 14, 2018

@PatrickvL I dropped the LOG_PREFIX parameter from the debug macros, but I can’t do the same with EmuLog and DbgPrintf because there are instances where they do not use the default LOG_PREFIX defined in the current translation unit (e.g.: EmuLog in WM_COPYDATA, in CxbxPopupMessage, in EmuXB2PC_D3DMultiSampleFormat, DbgPrintf in CxbxKrnl.cpp).

resource/Cxbx.rc Outdated
@@ -43,6 +43,22 @@ BEGIN
BOTTOMMARGIN, 207
END

IDD_CONTROLLER_CFG, DIALOG
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for add these with empty fields? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I didn’t do it, it must have been VS when I used the resource editor.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's leave it as is.

@RadWolfie
Copy link
Member

RadWolfie commented Aug 14, 2018

  • Include support of debug logs in release build.
  • Bug for "Emulator" event list. If I enable all, close emulator, re-open, open logging dialog window. Then SMC checkbox is unchecked.
  • Accept / cancel buttons are missing.
  • Keep log settings as static? I'm on 50/50 either static or runtime. ergo720 has a good reason, see his comment below.

Other than above fixes, there are some output (such as shader, old/new file paths, etc) shouldn't be in unfiltered log output. I think former developer(s) did this on purpose. We will fix any unfiltered logs later on.

@ghost
Copy link
Author

ghost commented Aug 14, 2018

@RadWolfie Here is a reason to have dynamic logs: what if you have a game with a bug of unknown origin and the relevant log doesn’t show up with the current settings? If the program doesn’t support changing logging settings at runtime, then you have to close it, change settings and relaunch it until the interesting log shows up, which can become annoying if you have to do this repeatedly or you have to play for a long time until you reach the actual section of the game with the bug. This is a typical use case of dynamic logging. With it, you can keep logging at a minimum when not needed and only increase it when you reach the affected area. The hotkey won’t help since it’s not specific and disables everything.

@gandalfthewhite19890404
Copy link
Contributor

There is 2 things (bug/feature) concerning logging stuff:

  • if title launch another xbe its completely delete all logged information.
  • we (testers and users) need logging indication

@RadWolfie
Copy link
Member

If I understand above comment...

  1. (bug) after title launch another xbe, the log settings are revert back to original settings.
    • Since the update didn't send the new change to EmuShared.
    • Plus it is best to send it to EmuShared, then send a message to child process to get new update from EmuShared.
  2. You should be able to see such as... (live example as of commit a5d97e6)
    [0x4CCC] WARN: NV2A    TODO: vbe_update_vgaregs
    [0x4CCC] WARN: X86     EmuX86_IOWrite(0x000080C0, 0x0001, 1) [Unhandled]
    [0x4CCC] WARN: D3D8    D3DDevice_SetRenderState_LineWidth not supported!
    
    Since NV2A, X86, D3D8 are log indication. Unless you meant something different?

@gandalfthewhite19890404
Copy link
Contributor

  1. If game launch another xbe - logfile is flushing and logging starts over - that is what I mean.
  2. logging indication - GUI indication that logging is on, its useful when you turned off log by hotkey.

@PatrickvL
Copy link
Member

This looks pretty solid now. What issues remain could be done later I guess?

@ghost
Copy link
Author

ghost commented Aug 15, 2018

There was a conflict before which should have been solved now. The other two problems mentioned by @gandalfthewhite19890404 can be done later as they have a separate issue.

* Reduce duplicate methods perform same task into one simple function.
* Use EmuShared to obtain updated log settings instead of WM_COPYDATA message.
* FIXED: Set log config for second GUI process
* FIXED: Proper update logging config to EmuShared instead of repeatly send when it is indeed in use.
  * Without this fix, it will affect other GUI process emulating a title.
@RadWolfie
Copy link
Member

I agree with @ergo720 since other two problems are not directly relative to this pull request, log filter feature, and are part of feature requests.

That's all the bugs I found with some clean up of duplicate methods. I can confirm new commit I made does work on my end even with second GUI process running.

@LukeUsher
Copy link
Member

This could be very useful for debugging scenarios without the log being polluted by other modules, thanks!
A little part of me has concern that it is a little bit too configurable, however, I feel that it could be confusing for users.

That said, I'm happy to merge

@LukeUsher LukeUsher merged commit f73114d into Cxbx-Reloaded:develop Aug 16, 2018
@ghost ghost deleted the logger branch August 16, 2018 10:05
@gandalfthewhite19890404
Copy link
Contributor

Awsome job, thanks!
Need to write manual for that.

@gandalfthewhite19890404
Copy link
Contributor

Hmm, still logging some file-related stuff, I set on only D3D checkboxes in logging settings. Its console output - easier to see.
image

@RadWolfie
Copy link
Member

We know about this, not all logs are being filtered. It is not affecting this pull request by the way. Even shader compiler are outputting too with log filter disabled.

LukeUsher added a commit that referenced this pull request Sep 6, 2018
Having multiple build types for each branch is proving to be confusing for some users. 

Ever since we implemented #1388, there has been no end-user benefit for using Debug builds, as they can get the same logging result by toggling the log level to Debug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement general improvement of the emu user interface GUI related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants