SCUMM: [RFC, WIP] Split enhancements into categories #5102
There was discussion a while back about how people may want to enable some of the SCUMM enhancements, but not all since some of them affect gameplay beyond simple bugfixing. This is an initial attempt at the GUI part of it. The details are very much still up for discussion.
Phase 2 will be categorizing the enhancements, and changing _enableEnhancements from a bool to an integer of some sort, possibly with some helper functions to make it easier to read.
See https://wiki.scummvm.org/index.php?title=SCUMM/Game_Enhancements for the current enhancements. Here's a rough idea of how I might categorize the optional enhancements. Again, this is open to discussion. I did this twice, and made some different decisions the second time! If you're interested, I'd suggest you do it blindly and compare your results to mine.
Zak McKracken and the Alien Mindbenders
Indiana Jones and the Last Crusade
The Secret of Monkey Island
Monkey Island 2: LeChuck's Revenge
Indiana Jones and the Fate of Atlantis
Day of the Tentacle
Sam & Max Hit the Road
The Curse of Monkey Island
First of all, great job on the PR. I think we definitely need to have separate classes for the enhancements, so I strongly support the work done here.
Here's my feedback:
As engineers and programmers, it's not always in our blood to be able to produce UX/UI which meets the needs of non-technical users; this is why I think we should think ahead for possible UI changes.
Let me explain myself better: right now we have three classes of enhancements (bugfixes, glitches and content); these ones are exactly the ones you are proposing to be shown up in the UI.
Why am I even thinking about this?
Let's say that we go with the dropdown list that you suggested for the UI. At some point in the future there might be someone very well versed in UX/UI (more than me, that's for sure...), who proposes a change. If we keep the current system, this change might even involve changing the names for the enhancement categories, or even change some of the enhancements themselves to be put in different classes than before. Don't get me started on configuration retrocompatibility 🙂
What am I proposing then?
These are my proposals for the enhancement classes (the code) and the enhancement groups (UI):
Each of the groups on the right activates the correct classes shown on the left.
What are we saving on the scummvm.ini files? I'm thinking we save bool flags for each of the classes on the left. Starting from an old config file with just the enable_enhancements flag set to true, we should parse that, delete said entry from the config, and produce all the class flags set to true.
Now, what should each enhancement group contain? Here's the description for each one of them:
As you might have noticed, I changed some names and added some others: I felt that having both "bugs" and "glitches" is too ambiguous: the end user is probably not technically versed, we should not expect them to know the difference and we should avoid confusing them, and therefore it's preferable that we produce very clear names.
Please let me know what you think in detail. I think that creating categories for the enhancement is both a very delicate operation and a very important thing to do for this engine, so I'm hoping we can all discuss this extensively before we commit to any choice.
P.S.: I'm willing to mark every single enhancement in the list with the appropriate class, since I proposed this fine-grained approach 🙂
Right now it's a single boolean. With this pull request, it would be a single integer with one flag per bit. The type for that is "int", so its actual size is unclear. I think we already assume it's more than 16 bits in the "output_rate" setting? Hopefully that's more than enough here.
I have no problem with that, since the current wording is just a placeholder as far as I'm concerned. Indeed, the pop-up itself is kind of a placeholder. It should be a fairly straightforward to change to createEnhancementsWidget() / addEnhancementsLayout() to replace it with something different. The function names were kept deliberately vague, and only the ScummOptionsContainerWidget class knows the specifics.
(The details are already starting to blur a bit in my head, but ScummOptionsContainerWidget is a class responsible for creating the enhancements and "original GUI" widgets, to help keep them consistent between the the SCUMM games that use the "standard" settings and the ones that need more elaborate settings, with sliders and whatnot, e.g. Loom.)
I look forward to that. I've only given your comments a quick read-through (I have to go to work soon), but it'll definitely make them easier to digest.
When you do, please note that the ScummVM wiki lists some enhancements that I didn't include in the above list, because they're currently always enabled. (It's also possible I missed some when I copied the relevant details, I haven't checked.) See https://wiki.scummvm.org/index.php?title=SCUMM/Game_Enhancements for details.
I believe those unconditional enhancements fall into two categories:
I don't know if anyone ever wants to actually make such enhancements conditional, but who knows... GSoC 2029: Implement all remaining original SCUMM bugs. :-)
This is an amazing PR. I can't tell you how many times I've heard people complain about ScummVM not being "faithful" to the original games (menus, bugs, and all), so it will be great to be able to put it all in the hands of the end user.
I do agree with AndyXP about the need to adapt it for the non-technical types because many if not most of the ScummVM userbase aren't especially technically minded.
Speaking from more of an art background than coding, I can see non-technical people having issues with categorizing these. I'd probably be confused about them as well had I used it without seeing the details of this pull request.
I think it's important to stress that keeping bugs is to have the game as close to the original as possible, otherwise, people wouldn't see the point of having bugs and would write it off as cruft.
So I think it would be better to spell it out so that people would see the utility of the option. Something like: Keep the bugs from the original game, Keep the glitches from the original game, Add cut or alternate content to the game.
These should not be compounded on each other either - ie: Keep bugs and glitches. It would be better to have them as individual options for checkboxes so that each user can pick and choose. Having them in a * and * setting would be way too confusing for the non-technical end-users.
Just to test my assertion that changing the widgets would be easy, I tried (but did not commit) replacing createEnhancementsWidget() and createEnhancementsLayout() like this:
To implement saving and loading, one would have to add variables that refer to the individual checkboxes, of course, but this is just an example. My main concern is that adding too many widgets will make the dialog hard to use, particularly at lower resolutions. (Side-by-side checkboxes is possible, but probably not a good idea at low resolutions. Also, they are hard to predict when their labels can be translated.) I don't know if we have the infrastructure to add yet another tab, and that too may get cluttered.
I guess it'd be possible to add a button that pops up a new dialog, perhaps with a text label on the side to describe the current settings?
Oh well, here's what the change above looks like when I run it. Perhaps this example will be helpful if anyone wants to make their own mock-up? I'm by no means an expert on what the theme engine is and isn't capable of:
It indeed seems simple to change and switch elements with the new generic ContainerWidget, so thanks for that!
Personally I'd vote for the checkboxes, as they offer a great degree of personalization for both standard end users and technical users. Like eriktorbjorn, I would also love to see other people (expecially people versed in art and graphics like Jen) proposing mock-ups to see both what the end user expects and how could we adapt that for low res displays.
Actually I was thinking of directly saving the "extended" classes in the config file, either via bit flags like you proposed, or via booleans.
Let's say I have the
Now, what would happen at the loading stage:
I see that this would complicate a little bit the saving/loading routine (I think we would actually need to subclass the
EDIT: Of course, any alternative to this saving/loading routine with ensures future compatibility would be more than welcome!
I meant something like
I'll be going on vacation soon which, counterintuitively, will probably leave me with less time for this. I'll try to check in on it from time to time, but I won't be able to act on it.
Of course, if by some miracle the details are nailed down in the next few weeks, I have no problems with anyone else putting the finishing touches to it. Yeah, yeah, wishful thinking. :-)
Heyhey! I'm gonna have quite some time in the mid of August to complete/help complete this. Do we have some other feedback (@dwatteau ?) or any other proposals? It'd be great to have everyone to approve a final proposal of requirements for this PR, so that I can begin helping with the actual coding.
Unfortunately, I don't think I'm going to have much free time until maybe late September :(
I like your idea of having a good granularity in the code, and I agree with JenniBee's suggestion of making sure the associated UI/UX remains accessible for non-technical users (e.g. add very obvious descriptions like "Fix original script bugs", "Fix unintended graphical issues", "Fix important usability problems", "Fix continuity errors", "Fix obvious spelling errors", "Restore original content hidden by script bugs").
While on the subject, I noticed this in the ScummVM NEWS.md file:
I guess technically that should also be an enhancement? I haven't checked how this is implemented, though.
...well, you do have a point. 🙂 I might need a second opinion on this one though.
This also removes the enhancement pop-up list which isn't needed anymore. For further information about enhancement groupings: scummvm#5102 (comment)
This also removes the enhancement pop-up list which isn't needed anymore. For further information about enhancement groupings: #5102 (comment)