#8353 closed patch
OSystem layer with bigger resolution
| Reported by: | sev- | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Ports |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Here I present OSystem change which were mentioned some time ago on the devlist.
It lets us to have that layer where cursor and menus live use 2x or 3x bigger size than original resolution. As it was discussed, it gives us possibilty to have 640x400 in-game interface as well as launcher.
It runs perfectly with 1x scale, i.e. with currenlty implemented behaviour and also it requires just one small change to backends which aren't going to implement it. All other functions have default implementation which runs layer in 1x mode.
Basically it adds another surface with keycolor transparency, draws cursor there, and works with GUI as former _tmpscreen, i.e. no changes to current gui code were made to make things run.
Thing gets turned on when showOverlay() is called. In this mode mouse events are generated in overlay space, i.e. 640x400 for 2x overlay.
OSystem changes: o initSize gets third parameter (default to 1), which specifies overlay scale factor. Currently overlay works only for 320xYYY modes, i.e. in 640x480 its scale is always 1. This is the only change which has to be implemented for other backends, but its value could be safely ignored there.
o OverlayColor overlayKeyColor(). Returns transparent color of the overlay. Is not used now, but may be if you want to draw holes.
o int Scr2Ovl(x) and int Ovl2Scr(y). Functions which convert coordinates between game and overlay. I.e. if you want to draw something on overlay but align it with game screen, use those.
o void cursorScaled(bool scale). If called with true (default), then cursor is always scaled to match current scale factor, i.e. it becomes bigger as in current implementation. When it is called with false, then cursor is used as is. It is used with HE 7.0 games which ran in scaled 320x200 with cursors designed for 640x480.
Also this patch introduces GF_UNSCALED_CURSORS which is set for HE 7.0 games.
I run launcher as well as all 320x200 SCUMM games with 2x overlay. As current GUI is 320x200-specific, it spans only on 1/4th of the screen, but this is out of this patch scope :).
This patch has some flaws: o It uses 1.5x scaler when user requests scale to 3x with 2x overlay, I wrote quick and dirty implementation of it, so it has to be replaced with something more nice looking.
o It obviously restricts scaling for rates lesser than overlayscale, i.e. for 2x scaler you cannot switch back to 320x200. But if you have some 1x scaler specified in your ini file, it doesn't avoid that and crashes. This has to be fixed in main.cpp
o For same reason when you try to start 640x480 game with 2x overlay from launcher, runGame() tries to switch to GFX_NORMAL before the game engine calls initSize, and this leads to crash. It is design flaw in current implementation, and probably such engines should check scale by themselves and call initSize appropriately.
o When overlay is visible, it copies game screen and uses it as an eyecandy by providing interface shadows. As there is 1.5x scaler introduced, simple scalers are used to produce shadows. There could be added some brains to call, say HQ2x in HQ3x mode, but I'm not sure is it really necessary.
o now showOverlay() and hideOverlay() functions change their meaning and could be renamed to activateOverlay() and deactivateOverlay() or some better names. I didn't do that in the patch.
o We could introduce kFeatureScaledOverlay or something to denote backends which support overlaying and fall back to 1x if they're not. But getOverlayHeight() could be used for that as well.
o Cursor jumps when user switches resolution. But it works in same way in current implementation, just becomes more obvious with this patch. It definitely has to be fixed.
Your feedback/critique/whatever is more than welcome.
Ticket imported from: #1013937. Ticket imported from: patches/458.
Attachments (10)
Change History (47)
by , 20 years ago
| Attachment: | overlay.diff added |
|---|
comment:1 by , 20 years ago
| Owner: | set to |
|---|
comment:2 by , 20 years ago
| Summary: | OSystem layers with bigger resolution → OSystem layer with bigger resolution |
|---|
comment:3 by , 20 years ago
Forgot to mention yet another flaw. Overlay ignores dirty rectangles, which is bad, but it is quite easy to add.
comment:4 by , 20 years ago
Quick question: why do you use a colorkey enabled surface for _overlayscreen ? So that the mouse cursor can be drawn in it? That's seems a bit like overkill, doing one additional full screen surface blit (with lots of keycolor data) while the overlay is hidden, just to draw a mouse cursor...
Maybe the whole mouse cursor handling should be rewritten, possibly independant of this patch... The current system is quite ugly, we draw the mouse cursor into the regular game screen space, backing up the data behind it -> ugly approach. Maybe we should get rid of drawMouse/undrawMouse. Instead, store the mouse data in a SDL_Surface (8bit, with colorkey, size matching that of the cursor). Then use SDL_BlitSurface() to blit it directly to _tmpscreen/_hwscreen (before or after scaling, depending on the needs).
by , 20 years ago
| Attachment: | Normal1o5x.patch added |
|---|
Slighly better 1.5x scaler (in theory, as it is untested)
comment:5 by , 20 years ago
The Normal1o5x scaler should have a check for width & height being even, else it might access memory out of bounds (as it reads 4 pixels at a time). It would also be trivial to make it look nicer via bilinear filtering. Something like the attached (completely untested) scaler code.
comment:6 by , 20 years ago
* do Scr2Ovl and Ovl2Scr really have to be exported? If so, I recommend that they are changed to follow our naming conventions, and also to convert a whole point at a time (that way, aspect correction etc. can be taken into account, too): Common::Point screenToOverlay(Common::Point p); Common::Point overlayToScreen(Common::Point p);
* Likewise cursorScaled() should be setCursorScaled(). The name is confusing, though; a better doxygen comment would be needed. Thinking about it, why not get rid of it and instead add another param to setMouseCursor() ?
* overlayKeyColor() -> getOverlayKeyColor()
* Specifying the overlay scaling factor directly maybe isn't the best solution. We are more interested in requesting a specific overlay size, not so much in which scale factor it has. Like, we may want to have an overlay of at least 640x400 pixels (if it's bigger, e.g. 640x480, fine). We don't care whether that is 1x, 1.5x, 2x compared to the game screen size, or the window size, or whatever...
So one might want to specify the overlay size directly. Of course that has its own drawbacks. I am not yet sure myself what I think is best, I just wanted to mention this here (you maybe already thought about all this yourself, too :-)
comment:7 by , 20 years ago
Regarding switching to GFX_NORMAL mode *before* the initSize(640,480) call -> yeah that causes a glitch now already, too, since it first changes the scale (so you briefly see a smaller window) before it changes the res. There is no good alternative to this right now; the only other way would be to call initSize first, but then you end up showing a 1280x960 window briefly. Ideally, it would be possible to switch the scaler and the screen size at the same time.
So to solve it, we have to extend OSystem. One approach would be to extend initSize to take an (optional) graphicsMode paramter. And an aspect ratio param, too. And maybe more :-). Or we add a "transaction" system to OSystem, during which GFX mode changes take no immediate effect. Consider this pseudo code to illustrate what I mean: g_system->startGFXTransaction(); g_system->initSize(640,480); g_system->setGraphicsMode(GFX_NORMAL); g_system->setFeatureState(kFeatureFullscreenMode, true); g_system->endGFXTransaction();
That would be more work for backends of course; but they could just provide dummy (empty) implementations for these new methods (so they would run identical to the current code). And actually implementing that shouldn't be too hard (e.g. it'd be fairly easy in the SDL backend).
P.S.: That talk about backends reminds me, you maybe should post a link to this tracker item to scummvm-devel and notify porters about it, so that they can comment, too.
comment:8 by , 20 years ago
I used colorkeyed surface to have more flexibility as for the interface itself. But with current approach of blendRect could solve it in some way. Also I think that mouse extension could go with this patch as well, I'll try to implement what you mention with cursors soon. With it we can forget about mouse grabs at all.
Fixed bug in your version of 1.5x scaler and use it. It's much better now.
Scr2Ovl and Ovl2Scr could be beneficial if somebody will use new layers for Kanji rendering in FM-TOWNS titles. As of making them operable on Point objects, that will make difficult use of most our graphics functions. We work with separated x and y (see newgui for zillions of examples and OSystem itself. Then those should be converted to use Point as well, otherwise we will stack lots of temporary objects/variables around their calls.
Speaking of aspect correction, currently engine knows nothing about it, just backends, and not every backend use it. First I used it, but quickly came to conclusion that it's better to leave as is, i.e. same scale at both dimensions and apply aspect correction to layer too.
setCursorScaled() as setMouseCursor() parameter. Now I use it at game detection stage, so later code is generic. But I'll think about it.
Transactions is a great idea. Reminds me OpenGL a bit ;) That will definitely resolve many issues.
Updated patch attached.
comment:9 by , 20 years ago
* ScreenToOverlay -> screenToOverlay OverlayToScreen -> overlayToScreen I recommend a look at <http://www.scummvm.org/ documentation.php?view=conventions> :-)
* I don't think we need any "flexibility" a color keyed surface provides; it just eats CPU power and does us no good. But if you can think of useful applications, please tell me.
* I see your problem regarding overlayToScreen; OTOH, I do not like it at all that a single version converts both X and Y coordinates. That seems very inflexible -- it means we have to use a fixed scale in both directions (making it hard to expose ration correction, should we ever want to) and also we can't do other fancy things, like shifting the overlay compared to the regular screen space. Granted, neither is something we need right now. But I am very touchy when it gets to clear design, and this just feels bad... but that's my guts only :-) BTW, I am all for converting the SDL backend code and the GUI code to make better use of the Point and Rect classes :-). They simply are older than Point/Rect, which is why they rarely use them.
* While it is true that currently the engines knows nothing about aspect ration correction (ARC), the same can be said about scaling -- the engine doesn't know whether it's run at 1x, 2x or 3x..... It certainly is *simpler* to not expose the ARC; at least when porting the existing code over. But it would also be beneficial to expose the ARC stuff. After all, ARC is there to make older games made for a screen with non-square pixels look correct on modern screens. But when applied to our modern GUI, it'll stretch something which was designed for square pixel devices; as a result, the GUI now looks wrong. So for this purpose, it would be preferable not to AR the overlay!
* I still would prefer if the cursor scale thingy was a param of setMouseCursor(). And the code would not be any more complicated either, you could still use GF_UNSCALED_CURSORS, but instead of calling setCursorScaled() once, you just modify the single call to setMouseCursor() ... patch has identical length/ complexity, but I'd be more happy about the design :-)
* GFX "transactions" indeed would be nice. See also the comment in OSystem_SDL::load_gfx_mode().
comment:11 by , 20 years ago
Well, let's postpone big patch for a while.
Here I attach first version of mouse-on-surface patch. Mouse here is remndered on a small keycolored surface which is then applied to hw surface.
Things which aren't implemented yet are:
o GF_SCALED_CURSOR in scumm.cpp for HE 70 games
o cursor scaling when user requests another scaler. Now
cursor
has fixed size always. It will require additional
temporary surface
as our scalers do not work with 8-bits sources, only
16-bits.
comment:12 by , 20 years ago
Regression with that patch: the mouse cursor is cut off prematurely if you move it close to the right or bottom border.
comment:13 by , 20 years ago
Another problem: hotspots are in the wrong "spot" when in 2x mode -- a guess would be that the hotspot is placed/moved as if the cursor was scaled (as it used to be), but of course since the cursor isn't scaled anymore, that's wrong.
comment:14 by , 20 years ago
Here is updated mouse patch with all mentioned problems fixed and features implemented.
Instead of GF_SCALE_CURSOR I define GF_2X_CURSORS and parameter to setMouseCursor as cursorTargetScale. It reflects its purpose more correct.
comment:15 by , 20 years ago
Sadly this patch doesn't apply against CVS anymore :-/. Of course I realize that it's near impossible to keep big patches in sync with the ever changing CVS. BTW, if you wanted to, you could easily create a CVS branch to work on this (I could assist with that, if you don't know how that's done), it might help in working on this.
As for the mouse scale problem: I wonder if we couldn't use the AdvMame2x scalers (possibly adapted versions) to scale the mouse -- they work fine for 8bit data, after all. And since they don't interpolate colors, the "transparency" of the mouse cursors shouldn't cause problems.
A little more work would be needed for the 1.5 case, of course... hm.
comment:16 by , 20 years ago
Here is updated patch for mouse cursors.
Now it features advmame2x as default scaler and result is really nice.
Also it includes that change I mentioned which lets to have cursors with different palette. It introduces new backend kFeature and if feature isn't implemented it falls back to b/w cursors.
Patch is against 2005-02-07 CVS. Please, review this patch. I think it's ready for commiting.
comment:17 by , 20 years ago
If used in The Dig, the patch produces a black pixel in the top left corner of the mouse cursor.
comment:18 by , 20 years ago
Seems to be AdvMame2x artefact. Will try to use transparent border around cursor. A dirty hack would be make that pixel alway transparent :)
comment:20 by , 20 years ago
FYI, AdvMame2x is working correctly, it just always works on an area which is 1 pixel bigger in all directions. As such, your solution is correct in so far as it increases the surface by 2 vertically and horizontally. However, you probably also want to init the whole surface (including the 1 pixel wide 'border) to _mouseKeyColor.
comment:21 by , 20 years ago
Well, that's exaclty how I avoid that pixel. See blitCursor() in backends/sdl/graphics.cpp.
How's the overall readiness of the patch?
comment:22 by , 20 years ago
The scaled mouse doesn't look that good, I think -- the mask isn't scaled correctly. In particular, I think it'll be necessary to make custom scale proc, based on the advmam2x scaler. The custom version would scale the image and the mask at the same time. That's the only way I can think of which will ensure the mask is correct...
comment:23 by , 20 years ago
There is _no_ mask. There are transparent pixels. If you mean that in some cases you see tiny black dots crowded horisotnally, that's 1.2 aspect ratio correction scaler. Only way to avoid that completely is to copy data from underlying image surface before scale, but that may be problematic, as will require to create yet another surface. Now we do final scale directly to _hwsurface and there is no copy of non-corrected surface. That will add 2 additional blits and I think will be a noticeable slowdown.
So I assume I have to give up and discontinue this patch.
comment:24 by , 20 years ago
I don't think you have to give up now, you are almost there, IMO...
You are of course right, there is no mask -- I simply mixed this up, sorry. Of course you are right, the aspect scaler messes this up. Now, if modify common/scaler/aspect.cpp to use the kVeryFastAndUglyAspectMode mode, the cursors look pretty good. Maybe we could just make a custom version of stretch200To240() which always uses that mode; or turn the aspect mode into a template parameter of the stretch200To240 function, or something like that...
comment:25 by , 20 years ago
Here it is. Cursors really look nice without any artefacts now. Ugliness is unnoticeable :).
comment:26 by , 20 years ago
Great work. Now that both the transaction stuff and the scaled mouse code are in CVS, we can even consider turning back to the original purpose of this patch tracker item -- improved overlays ... :-)
comment:27 by , 20 years ago
With all the changes to the cursor handling, perhaps this is a good time to bring up bug #1022283 ("ALL: Wrong cursor colour in NewGui")? I noticed that the cursor palette feature does not currently apply to NewGui's cursors, though.
comment:28 by , 20 years ago
Hm, I noticed one potential problem with the setCursorPalette API. There is no way to disable the custom mouse palette if it has been used once. That's a problem if you need to switch between custom and non-custom mouse palettes -- e.g. because you first play a HE game, then return to the launcher and start another game.
So the API maybe should be enhanced by a disableCursorPalette() method...
comment:30 by , 20 years ago
Finally here is third version of overlay patch. It addresses all discussed issues and is much simpler because of transactions and separate mouse surface which are both present in CVS now.
o Currently I still uses overlay scale, not specified size, i.e. in usual 320x200 case it will be 2x, i.e. 640x400 o It does not let to switch to resolutions lower than overlay size.
GUI now uses _overlayScale to determine either scaling required, so it looks nice now with 2x overlay until we rewrite whole GUI with use of finer resolution.
2x overlay is specified by SCUMM and GUI.
A lot of time was spent to resolve off-by-one errors mainly caused by 1.5x scaler and our 1.2x aspect ratio correction function, so case when 1.5x scaler is used now threated differently in many places. Without it cursor produces garbage and backgound shakes.
Please, comment on it.
comment:31 by , 20 years ago
I commited fix to 1.5x scaler which is part of v3 patch. There was bug which led to endless loop with odd height value.
comment:32 by , 20 years ago
Looks pretty good now.
One regression: When you start comi directly, it'll switch comi to 2x scale, even though I configured it to be at 1x... I assume that is so because you changed scumm.cpp to always use 2x mode for the overlay? Just guessing.
comment:33 by , 20 years ago
No, this is because that restriction which doesn't allow switching to scales with resulting resolution lesser than overlay size. Present time we check for DEFAULT_TO_1X_SCALER in initCommonGFX() when screen width isn't set yet. Albeit thanks to transactions, additional check against 1x scaler and forcing it later resolves the problem.
Right now I do not let layers scale for resolutions other than 320xYYY.
Updated patch attached. Only minor changes in scumm.cpp.
comment:34 by , 20 years ago
Guess this is good enough now that we can continue working on it in CVS...
comment:35 by , 20 years ago
| Status: | new → closed |
|---|
comment:36 by , 20 years ago
Commited :) Also updated all ports, so port maintainers shouldn't worry.
comment:37 by , 6 years ago
| Component: | → Ports |
|---|
Scaled overlay patch