#8926 closed patch
SDL: Backend transaction / rollback support
| Reported by: | lordhoto | Owned by: | lordhoto |
|---|---|---|---|
| Priority: | normal | Component: | Ports |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
This patch adds proper transaction support to the SDL backend and also adds support for rollback to the backend. Rollback means if a newly set configuration, done inside a transaction block, can't be setup correctly it will try to switch back to the former configuration.
What needs to be discussed: - Somehow endGFXTransaction needs to return a error code on failure, for example if a new mode can't be setup so the client code is able to take care of it
Things that need to be reflected in common OSystem documentation: - Now setFeatureState called with kFeatureFullscreenMode and kFeatureAspectRatioCorrection needs to be called within a transaction block - Now setGraphicsMode has to be called inside a transaction block
- Porters subclassing the SDL backend should need to update their code, it would be wise to hear what they think of it
- Engines / other code calling setFeatureState and/or setGraphicsMode might be updated to do it inside a gfx transaction block
Things this patch breakes: - '-f' switch, since code in base/main.cpp will setup the fullscreen mode specified in the config file after transient domain is cleared. - '-g' switch for the same reasons as with '-f' as I would say, but I'm not really sure how this is done really (a.k.a: CALL FOR HELP, please somebody check this!)
Both are related to bug #2120780 "GUI: gui-theme command-line option ignored" as far as I can tell it. At least it has the same reason, that the transient domain is not kept when code relies on it.
What is tested: - I was able to switch scalers on run time, fullscreen toggle and aspect ratio toggle - Rollback should work, it successfully switched back from 3x to 2x mode when 3x is not supported by the system (tested with Xephyr)
It is of course still heavy work in progress as you see from the description.
Ticket imported from: #2123680. Ticket imported from: patches/1031.
Attachments (5)
Change History (17)
by , 16 years ago
| Attachment: | sdl_transaction_rollback_v1.patch added |
|---|
comment:1 by , 16 years ago
You fixed a typo in there: "Nomral1x sclaler" -> "Nomral1x scaler" Great, just go on and fix the other half, too *ggg*
Sorry, no real feedback yet.
comment:4 by , 16 years ago
All in all, this looks good to me. Some specific remarks / replies:
* endGFXTransaction() needs to return an error code: Well, I agree in so far that we should have a way to detect failed transactions. Right now, this will be "signaled" by a crash :). Adding a way for client code to detect and deal with problems (e.g. by showing an error and return to launcher / quitting) would be good (even though it might take some time for all engines to catch on). But it should also be possible to determine the cause of the failure, to decide whether to go on (e.g. if we fail to enable aspect correction, that's not a reason to quit), or whether it's critical (like not being able to set the right resolution). One way would indeed be to add an error value to endGFXTransaction() (sufficiently rich to let the *engine* determine whether it can go on). Since this affects all ports and to a degree all engines, this should be brought up on scummvm-devel.
* Likewise, the ports subclassing the SDL backend indeed need to be informed -- this would be handled by a scummvm-devel mail, too :)
* Likewise, everybody needs to be told about the new requirement for setFeatureState (well, for some "features") and setGraphicsMode to be called inside a GFXTransaction -> scummvm-devel
All in all, I like the patch, but scummvm-devel needs to be informed.
comment:5 by , 16 years ago
Since no feedback (code wise that is, since Begasus wrote at least a mail) I added a simple OR flagged error variable return to OSystem::endGFXTransaction and documented it accordingly. I also changed the rollback support to first revert fullscreen, then aspect, then gfx mode and then gfx dimensions. Some more in depth thinking about this might be nice here as in the order of reverting, or trying to keep as much as possible from new mode definition.
Patch against current SVN is added, only SDL backend is updated currently. So the rest of our code base doesn't take any advantage of the new error return value of OSystem::endGFXTransaction. File Added: sdl_transaction_rollback_v3.patch
comment:6 by , 16 years ago
It seems I forgot to finish my first sentence of the last comment:
"Since no feedback (code wise that is, since Begasus wrote at least a mail) has been given, I added ..."
I had another idea right now, when we switch to different screen dimensions, we could actually try to lower the scaler factor on failure, so people accidentally setting their scaler to an incorrect value, could still be able to use say the launcher (I remember some people on the forums for whom 3x wouldn't work, but they set it up by accident, so they wouldn't be able to use the launcher anymore). Just a (maybe stupid :-) idea though.
Of course that could now be caught via a immediate mode switch in the launcher and checking the error code of endGFXTransactions. It would be limited to global scaler selection though.
comment:7 by , 16 years ago
I checked out our gfx transaction use a little bit and I guess it should be quite trivial to convert the engines to use the new error flag.
Basically I guess we could just add some new function which sets screen resolution and automatically fails on error and convert engines to use that. A possible place would be inside Engine or to the global namespace like initCommonGFX. With that we can simply catch errors on resolution change and are still able to print a proper error message to the user instead of possible crashing inside OSystem somewhere later on.
Maybe you have some thoughts on that one, apart I would say the patch should be in a ok to good condition for trunk. At least I sadly don't expect any feedback from porters before we broke the compiling of their ports :-/.
comment:8 by , 16 years ago
I added a simple fix which prevents the rollback from trying to fall back to the old mode in an endless loop when even the old mode couldn't be setup anymore in certain cases.
comment:9 by , 16 years ago
OK I now added a simple function "initGraphics" in engines/engine.h which takes care of proper error handling (it displays a warning to the user on various endGFXTransaction errors and fails if the size change failed.
I also adopted all engines to use it.
comment:10 by , 16 years ago
Much nicer.
For a moment I wanted to complain that it shouldn't error out when it can't set a res, but instead should give the engine an opportunity to react, e.g. by returning the launcher. While that might be nice, on second though I don't find it that important. Do we really have unsupported resolutions that often? Maybe something for a future revision... For now I wouldn't mind if you commited this, I guess. Though maybe a last notice to scummvm-devel would be good, so that porters can't complain that they were not notified in time *gg*
comment:11 by , 16 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → closed |
comment:12 by , 6 years ago
| Component: | → Ports |
|---|
Patch against trunk r34628