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

ENGINES: Standardization of debuggers #2052

Merged
merged 73 commits into from Feb 16, 2020
Merged

ENGINES: Standardization of debuggers #2052

merged 73 commits into from Feb 16, 2020

Conversation

@dreammaster
Copy link
Member

dreammaster commented Feb 9, 2020

Since my previous pull request for autosaves is touching so many engines anyway, I thought it an opportune time to deal with another minor issue I've been seeing for a while, that of debuggers.

The current status quo is that engines would create the debugger themselves, and then be responsible for detecting arbitrary keypresses (normally Ctrl-D), and manually attach to and open the debugger. Plus, they also needed to regularly call the debugger's onFrame. With this, the bulk of that is done away with. Engine's simply instantiate a descendant of GUI::Debugger and call the new Engine method setDebugger. The core engine & graphics managers then take care of detecting Ctrl-D to open the debugger, and regularly call it's onFrame.

So this has two benefits:

  1. It's easier for engines to use. They merely need to create their debugger and pass it to the engine. No more mucking around detecting keypresses, and having to do things like g_vm->_debugger->attach from their event manager classes.

  2. It seems some engines weren't remembering to override the getDebugger() method that Engine already had. This meant that when an error occurred, those engines would completely exit, rather than opening the debugger to show users the error message like other engines do.

Miscellaneous notes:

  • sci is the one engine that I haven't converted, due to it's more complicated debugger code

  • When a Ctrl-D or an error occurs for an engine without a debugger, Engine now creates a bare-bones debugger automatically. This means that any engine that causes an error now will result in the debugger window opening, whether or not the engine explicitly created one. Rather than just exiting the application entirely. So long as initialization was far enough along for g_engine to have been set

@@ -109,6 +110,14 @@ bool DefaultEventManager::pollEvent(Common::Event &event) {
// making invalid assumptions about ascii values.
event.kbd.ascii = Common::KEYCODE_BACKSPACE;
_currentKeyDown.ascii = Common::KEYCODE_BACKSPACE;

} else if (event.kbd.keycode == Common::KEYCODE_d && (_modifierState & Common::KBD_CTRL)) {

This comment has been minimized.

Copy link
@bgK

bgK Feb 9, 2020

Member

I would prefer if we did not re-introduce hard-coded key mappings in this class so soon after I made the effort to remove the existing ones 😉

I guess the way to go to get rid of it to add a new event type EVENT_DEBUGGER or something; add an Action binding that event to CTRL+d by default in the default engine keymap (and modify the existing actions in the per-engine overrides); handle that event here.

This comment has been minimized.

Copy link
@tsoliman

tsoliman Feb 9, 2020

Member

Maybe change the default shortcut to CTRL-SHIFT-D so as to not clash with the SCI games (like QFG games using it as a show-date)

This comment has been minimized.

Copy link
@bgK

bgK Feb 9, 2020

Member

Another option is for SCI to define its own engine keymap and bind the debugger to CTRL+SHIFT+d. That way people's habits are not disturbed.

This comment has been minimized.

Copy link
@ZvikaZ

ZvikaZ Feb 9, 2020

Contributor

I've recently played Loom with my 10 years old son. He used Ctrl-f few times to make things faster and slower, and was frustrated when he accidentally pressed Ctrl-d and got into the debugger.
Since the debugger isn't intended for the users, maybe it makes sense to make it less accessable, and make ctrl-shift-d global default for all engines.
On the other hand, it's difficult to change old habits...

Another idea, maybe have an "easier" default (ctrl-d) for debug binaries, and "complicated" (ctrl-shift-d) default for release binaries.

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 9, 2020

Author Member

I guess the way to go to get rid of it to add a new event type EVENT_DEBUGGER or something; add an Action binding that event to CTRL+d by default in the default engine keymap (and modify the existing actions in the per-engine overrides); handle that event here.

Not a bad idea, and as suggested, it could be bound to Ctrl-Shift-D to avoid conflicts with any games. I presume that the code needed for the keybinding is in I'll admit I haven't really been follow the recent keybinding discussions. Is this something already in master, and if so where do I go about setting up the binding.

This comment has been minimized.

Copy link
@bgK

bgK Feb 10, 2020

Member

Is this something already in master, and if so where do I go about setting up the binding.

Yes, keybindings are mostly complete in master. There are essentially two options for the debugger keybinding:

  1. We agree to use CTRL+SHIFT+d as the default shortcut for all the engines, and that engines cannot decide to use another default shortcut (some people might not like the furniture being moved around). In that case the keybinding can be defined right in this class, in the getGlobalKeymap method.
  2. We want engines to be able to set different default keybindings. In that case the keybinding needs to be defined in MetaEngine::initKeymaps, and all the per-engine overrides need to be adjusted to account for the new event type.

If you want to catch up on the recent keymapper changes, this page has a bit of documentation: https://wiki.scummvm.org/index.php?title=Keymapper.

This comment has been minimized.

Copy link
@dreammaster

dreammaster Feb 10, 2020

Author Member

Thanks. I'll have a review of it tomorrow night. Getting kind of late here.

@dreammaster dreammaster force-pushed the dreammaster:debugger branch from f8868ef to 535ea82 Feb 9, 2020
@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 13, 2020

Are there any pending tasks regarding this pull request? If not, it can be merged this Sunday, the 16th of February

@dreammaster

This comment has been minimized.

Copy link
Member Author

dreammaster commented Feb 13, 2020

The only other thing that comes to mind is the sci debugger triggering code still being engine specific. I'll take another look at it in the next day or so, but otherwise, it may be better reviewed by a more experienced sci dev post-merge

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 16, 2020

I see you fixed the pending issues with the SCI debugger.

Overall, great work, well done!

Merging as promised

@bluegr bluegr merged commit ba147f5 into scummvm:master Feb 16, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sluicebox

This comment has been minimized.

Copy link
Member

sluicebox commented Feb 16, 2020

I can't activate the SCI debugger anymore. Windows build with both visual studio and msys2.

@antoniou79

This comment has been minimized.

Copy link
Contributor

antoniou79 commented Feb 16, 2020

I can't activate the SCI debugger anymore. Windows build with both visual studio and msys2.

The default key combo for the debugger is now Ctrl+Alt+D (it's in the Global Options -> keymaps). Maybe that's the issue? (It's the one I had :) )

@sluicebox

This comment has been minimized.

Copy link
Member

sluicebox commented Feb 16, 2020

Thanks! That was it, I didn't realized it changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.