-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fix some bugs mostly related to classic/vanilla Doom behavior #28
base: master
Are you sure you want to change the base?
Conversation
gaborbata
commented
Oct 10, 2023
- Fix vanilla mouse event handling (in game, in menu)
- Fix initial value of novert (i.e. disable vertical movement by default)
- Fix fullscreen resize options (Nearest, Bilinear, Bicubic)
- Fix writing alternate config files i.e. when -config argument is used
- Fix/suppress some compile warnings, remove deprecated API usage
- Show message dialog on missing WAD files
- Improve logging
- Improve WAD handler cleanup
- Center application window
- Minor code cleanup and formatting
I'm looking at the changes this time, and unlike the previous MR, there are just formatting changes to files, like replacing tab indentation with spaces. GitHub displays 425 changes. This obfuscates your functional changes and it makes it hard to see which files are really affected. I don't like reformatting, but I'll accept code cleanup if it's done manually and not by a tool that does it automatically. Can you avoid formatting changes? It makes MRs hard to review. I prefer atomic commits: Atomic commits:
Your changelog is already well separated into different bullet point. Your commits should be the same. So one commit that does "Fix vanilla mouse event handling", one that's "Fix initial value of novert", another one that's "Fix fullscreen resize options", etc. One commit per bug fix, improvement or new feature. This would allow me to review much quicker. |
Thanks for your comments. Will try to separate the PR to multiple commits soon. (Please note that GitHub can ignore whitespaces during reviews) |
* Map keyboard arrow keys to numpad keys, as the underlying engine use numpad for movement. * Add classic.cfg with configuration for classic Doom keyboard control.
* Fix overwriting default.cfg with empty content when -config parameter is used.
* Mocha Doom loads wad files temporarily to detect Ultimate Doom, which can be closed after the detection.
* Apply missing rendering hints * Fix typo in Bicubic interpolation name * Set Native fullscreen mode the default, as Best mode is not stable * Properly show/hide cursor on focus lose/gain
* Fixed default novert value: vertical mouse movement is disabled (novert=true) by default, as in vanilla (and in other games as well). * Fixed mouse button order * In menu, right click now properly goes back to previous screen * In game, clicks are handled properly (left: fire, right: strafe, mid: forward) * Take use_mouse config value into account in menu as well
* Fix left/rigt key scan codes in default * Some code cleanup
Hey, @AXDOOMER I've split the changes into separate and meaningful commits. Unfortunately, I can't avoid the formatting changes (401d4ed) at that point easily, however this is a separate commit. I think a consistent (and standard) code formatting can help understanding and maintaining the code (and even to avoid some bugs). |
Remove some unused imports
Sorry for not having merged this yet. I haven't forgotten about it and it will get merged within the next few months. I'm just too busy with full-time work + full-time studying. |