Opened 15 years ago

Closed 15 years ago

Last modified 12 months ago

#8353 closed patch

OSystem layer with bigger resolution

Reported by: sev- Owned by: fingolfin
Priority: normal Component: Ports
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)

overlay.diff (30.1 KB ) - added by sev- 15 years ago.
Scaled overlay patch
Normal1o5x.patch (1.8 KB ) - added by fingolfin 15 years ago.
Slighly better 1.5x scaler (in theory, as it is untested)
overlay.v2.diff (32.8 KB ) - added by sev- 15 years ago.
Version 2 of scaled overlay patch
mouse.v1.diff (18.3 KB ) - added by sev- 15 years ago.
Mouse on a separate surface v1 patch.
mouse.v2.diff (21.9 KB ) - added by sev- 15 years ago.
Mouse on a separate surface v2 patch
mouse.v3.diff (25.1 KB ) - added by sev- 15 years ago.
Mouse on a separate surface v3 patch
mouse.v4.diff (25.7 KB ) - added by sev- 15 years ago.
Mouse on a separate surface v4 patch
mouse.v5.diff (26.4 KB ) - added by sev- 15 years ago.
Mouse on a separate surface v5 patch
overlay.v3.diff (25.2 KB ) - added by sev- 15 years ago.
Scaled overlay v3 patch
overlay.v4.diff (25.0 KB ) - added by sev- 15 years ago.
Scaled overlay v4 patch

Download all attachments as: .zip

Change History (47)

by sev-, 15 years ago

Attachment: overlay.diff added

Scaled overlay patch

comment:1 by sev-, 15 years ago

Owner: set to fingolfin

comment:2 by sev-, 15 years ago

Summary: OSystem layers with bigger resolutionOSystem layer with bigger resolution

comment:3 by sev-, 15 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 fingolfin, 15 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 fingolfin, 15 years ago

Attachment: Normal1o5x.patch added

Slighly better 1.5x scaler (in theory, as it is untested)

comment:5 by fingolfin, 15 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 fingolfin, 15 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 fingolfin, 15 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.

by sev-, 15 years ago

Attachment: overlay.v2.diff added

Version 2 of scaled overlay patch

comment:8 by sev-, 15 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 fingolfin, 15 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:10 by fingolfin, 15 years ago

Any news on this?
Esp. the mouse code patch would be interesting ;-)

by sev-, 15 years ago

Attachment: mouse.v1.diff added

Mouse on a separate surface v1 patch.

comment:11 by sev-, 15 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 fingolfin, 15 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 fingolfin, 15 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 sev-, 15 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.

by sev-, 15 years ago

Attachment: mouse.v2.diff added

Mouse on a separate surface v2 patch

comment:15 by fingolfin, 15 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.

by sev-, 15 years ago

Attachment: mouse.v3.diff added

Mouse on a separate surface v3 patch

comment:16 by sev-, 15 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 fingolfin, 15 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 sev-, 15 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 :)

by sev-, 15 years ago

Attachment: mouse.v4.diff added

Mouse on a separate surface v4 patch

comment:19 by sev-, 15 years ago

Here it is updated. Now that black dot is gone.

comment:20 by fingolfin, 15 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 sev-, 15 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 fingolfin, 15 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 sev-, 15 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 fingolfin, 15 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...

by sev-, 15 years ago

Attachment: mouse.v5.diff added

Mouse on a separate surface v5 patch

comment:25 by sev-, 15 years ago

Here it is. Cursors really look nice without any artefacts
now. Ugliness is unnoticeable :).

comment:26 by fingolfin, 15 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 eriktorbjorn, 15 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 fingolfin, 15 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:29 by sev-, 15 years ago

disableCursorPalette() is in CVS

by sev-, 15 years ago

Attachment: overlay.v3.diff added

Scaled overlay v3 patch

comment:30 by sev-, 15 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 sev-, 15 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 fingolfin, 15 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 sev-, 15 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.

by sev-, 15 years ago

Attachment: overlay.v4.diff added

Scaled overlay v4 patch

comment:34 by fingolfin, 15 years ago

Guess this is good enough now that we can continue working on it in
CVS...

comment:35 by sev-, 15 years ago

Status: newclosed

comment:36 by sev-, 15 years ago

Commited :) Also updated all ports, so port maintainers
shouldn't worry.

comment:37 by digitall, 12 months ago

Component: Ports
Note: See TracTickets for help on using tickets.