Experimental HOM shimmering effect implementation (SDL2)

[nukeykt]

https://github.com/nukeykt/chocolate-doom-beta-opl/tree/dirtybox
Edit:
Updated link

[fabiangreffrath]

Thanks for that, but... phew, three frame buffers only to somehow "better" emulate a bug/feature that is already there.

[linguica]

Vanilla DOS Doom was triple buffered, right? So I don't see why triple buffering Chocolate Doom would somehow be bad.

In fact, I'd even call it a good idea, since the entire point of calling them "HOMs" was based on the very distinctive shimmering visual effect, which Chocolate Doom doesn't recreate.

[AXDOOMER]

Nice, now I'm gonna be able to play wow.wad with 100% faithfulness.

(can be closed: #117)

[jmtd]

Yeah, sometimes I wonder how crazy I/we are with emulating vanilla bugs, but the HOM was a particularly distinctive part of the original Doom so I think we need to do the same buffering to get the effect.

(Actually I'm increasingly wondering if we're barking up the wrong tree with the whole approach to vanilla compat with memory bugs. I started to wonder whether we should be doing some kind of weird hybrid between a) targetting actual, real DOS with the engine and b) forking dosbox into a library/wrapper, stripped down, streamlined and customised for the sole purpose of running a), so that we have the whole DOS memory model folded into our emulation, but could still potentially do nice things like the video scaling, modern/hotplug input devices, TCP/IP etc.))

[haleyjd]

I'll note that with certain combinations of hardware/video driver/screen mode, you can actually get real HOM flickering in OpenGL if only parts of the screen are updated.

[fabiangreffrath]

Vanilla DOS Doom was triple buffered, right? So I don't see why triple buffering Chocolate Doom would somehow be bad.

I didn't mean to say that it was "bad". But I wondered if the only real advantage that this rather invasive patch brings is the somehow "better" emulation of a bug (or call it "feature", if you like) that is already there in Choco, though not emulated that faithfully.

[linguica]

The Linux Doom port did away with triple buffering, and Heretic et al all
used plain Mode 13h, right? So we don't have the actual screen drawing code
and anything related to keeping track of 3 screen buffers (if not four, at
least according to some stuff I read...). If we had it, recreating the HOM
effect would be the most natural thing in the world. But since we don't,
someone has to recreate it from scratch, so it feels unnatural, even though
it's really making the port more faithful.

On Thu, Mar 10, 2016 at 9:01 AM, Fabian Greffrath notifications@github.com
wrote:

Vanilla DOS Doom was triple buffered, right? So I don't see why triple
buffering Chocolate Doom would somehow be bad.

I didn't mean to say that it was "bad". But I wondered if the only real
advantage that this rather invasive patch brings is the somehow "better"
emulation of a bug (or call it "feature", if you like) that is already
there in Choco, though not emulated that faithfully.

—
Reply to this email directly or view it on GitHub
#676 (comment)
.

[jmtd]

Well, I can't comment on the code yet (haven't read it thoroughly) but I did built it just now and give it a spin, and it looks pretty damn good to me. (I tried http://doomwiki.org/w/images/b/b0/D218hom.lmp which is a good quick way of seeing HOM in Doom 2)

(I've been wanting to have a "bug missing" label ever since I created "feature removal", finally :D)

[jmtd]

@linguica: keeping track of the four 'screens' was probably not as much of a complex nightmare as you might imagine. IIRC each pair of screens were contiguous in memory, so it was only a touch more difficult than standard double-buffering.

[linguica]

Is it four? I tried recording in DOSBox and I only counted three images repeating in the HOM.

[AXDOOMER]

Using page-flipping, a program in mode 13h will have 256KB of VRAM available in divided in 4 banks. It's intriguing that they used 3 frames. They could have use 4 to fill the whole VRAM or they could have only used 2 and it would have got the job done. I don't believe using 3 buffers instead of 2 made anything faster.

[jmtd]

I always thought the strange buffer arrangement was necessary for the wipe effect. It's possible they needed 3 buffers for that and not 4, so there'd be no point using 4 just for symmetry's sake.

[jmtd]

I've just tried to read this a bit, in order to provide some feedback and hopefully increase the chances of this being merged soon. However about half way through I got hopelessly lost. I think I need to spend some time unpicking sdl2-branch first, to understand how that works, before I continue.

One comment, however. As Fabian says, this might be more of an "invasive" change than is necessary. If I were trying to implement this, my (naive?) approach would be:

• I'm assuming I_VideoBuffer is a software buffer and not allocated in video memory at all. This appears to not be true in some circumstances.
• add accessor routines for reading or writing to I_VideoBuffer and replace all uses of it to use those methods exclusively; one set of accessors in particular to be used only by the blitting process (this could be done in a way which used the compiler to help you by not making the buffer pointer exported in the .h then working through the compiler errors)
• replace the initialisation so it is 3 times larger (in my ideal world it would be statically initialised)
• adjust the accessors so they a) access a pointer into the memory region b) increment/wrap the pointer around the 3 regions via the blitting accessors

In particular that would mean the SDL-specific video code wouldn't need to be touched much by this feature, you wouldn't need to do that palette-setting stuff three times,etc. But I am perhaps labouring under a misapprehension since it seems I_VideoBuffer can be a native surface. Could that be a case of premature/unnecessary optimisation?

[haleyjd]

It does appear that the fourth page of VRAM is skipped:

  v6 = destscreen + 0x4000;
  destscreen += 0x4000u;
  if ( v6 == 0xAC000 )
    destscreen = 0xA0000u;

This will wrap back to page 0 when it is positioned at a page 3. I wonder if this was a bug.

[nukeykt]

No. That's not a bug. 4th page is used for storing status bar border(like a background_buffer in chocolate doom) and for disk icon.

[haleyjd]

You are thinking of the software screens array. The code I pasted above is dealing with the low-level VGA memory, which is non-planar (Mode Y is unchained; the writes in I_UpdateBox must be staggered every 4 bytes). Unless you are saying that one entry of the screens array is hacked to point directly into VRAM. I never noticed that during disassembly if so.

[jmtd]

Whilst waiting for feedback on #691, I thought I'd take another look at this. I thought it should be possible to make this much less intrusive by not touching any of r_draw, and instead moving the I_VideoBuffer pointer around. By making the bottom-most SDL_Surface 3 times as large, one can avoid having to allocate three surfaces, set the palette three times, etc. I managed to do all that tonight (wip here) but it doesn't work properly because in my tiredness I forgot to write to a different buffer to active (I.e. the fundamental rule of double or triple-buffered drawing, heh). So instead of HOM where it should be, you get HOM everywhere, all the time. Still that should be a simple fix when I'm more awake, and hot damn it is much less intrusive (32 insertions, 3 deletions, one file modified only)

[jmtd]

OK; I see why you've had to touch r_draw. I hadn't realised the y-lookup tables were calculated based on the value of I_VideoBuffer. I do have a question, though, why bring back the dirty-rects/I_UpdateBox/I_UpdateNoBlit system? Is that necessary for the HOM to look right?

[nukeykt]

I'm not sure about dirtybox. I just tried to write video code close to vanilla as possible to achieve accurate hom.

[nukeykt]

Dirty rects actually are important. While in game only status bar is updated in vanilla due to dirty rect mechanism,(except automap mode. Note that the text and menu are rendered by V_DrawPatchDirect, which renders them directly to video memory and not affects to dirty rects) so level "holes" are actually never updated, which causes HOM effect. If we updated the entire screen, we would get the static HOM even with the three buffers.

[nukeykt]

Check out this video:
https://youtu.be/rLHDM6dZW6A

[jmtd]

Folks, I've added this to the SDL2 milestone: you might not agree, but I'd like to see some more attention to this work since it's going to become much harder to merge as sdl2-branch keeps evolving.

[fabiangreffrath]

If you want it to get more attention, fine, that's one thing. But I don't believe that adding it as a blocker for the sdl2 migration is the right thing to do. If the hom-branch depends on the sdl2-branch, we should pretend it was the other way round.

[jmtd]

Consider it my vote that this should be part of the v3 release.

[fabiangreffrath]

we should pretend it was the other way round.

s/should/should't/

[jmtd]

I realised last night I probably haven't explained my feelings on this bug properly, so I'll have another go.

IMHO, implementing a more accurate HOM is a worthwhile feature for choco, since it was such as distinctive and common bug in Doom.

There are two things that jump out to me about this implementation. Firstly, it's a big chunk of work. As a free software project, I think we should encourage contributions, and to get a big contribution like this is a really good thing, and we should try to ensure that people like @nukeykt are valued and comfortable contributing.

The second is how invasive it is. I guess, although nobody else has said it, that this is one reason that no other reviewers have given this much attention. The situation has improved a little (look at 140514a). I think that it might be possible to rework the r_draw changes so ylookup is not turned into a 3x array but pointer arithmetic is used to get the right draw surface, which should make it less invasive.

The reason this particular PR jumped to my attention in recent times is because we've just merged further changes to the diskicon, which was fixed as part of this PR anyway. So we're not just ignoring this PR, we're actively redoing stuff it fixed, and making this harder to merge as we go. That upset me a bit, because I think it sends the wrong message to contributors. Since the diskicon changes are not the headline change here, I suppose it's possible you folks didn't actually realise that they were in this PR along with the HOM change.

So I suppose I'd like to know two things

1. does anyone else agree with me that we should have a more vanilla-accurate HOM
2. Assuming 1. is "yes", what needs to be done to make this mergeable?

But I think it's only fair to @nukeykt that we get a conclusive idea about these before we iterate further and further away from the merge base.

[haleyjd]

One comment right off the bat is what about Heretic and Hexen? They didn't run in Mode Y, they ran in vanilla 13h, and they didn't use the same dirty rects system either. I don't see that being accounted for here.

Personally, my feeling on the matter is that it likely degrades performance just in order to approximate something that was only an emergent feature of the program interacting with the VGA hardware, and not of the DOOM engine intrinsically. Versions of Doom on other platforms (NeXTStep, Linux, BeOS, Macintosh, etc) never had the effect. Is Choco meant to be emulating DOOM, or emulating a DOS machine?

[AXDOOMER]

Choco doesn't emulate the DOS machine. Choco is meant to be emulating DOOM like it would run in a DOS machine, because DOOM running in a DOS machine is vanilla. Other platforms don't have the bug because they do not run the vanilla engine. They can't because it only runs in DOS. The behavior of some bugs is even dependent on DOS and Choco tries to emulate them as closely as possible.

[nukeykt]

Timedemo with HOM emulation exits with the same fps as standard sdl2 branch.

[fragglet]

So there's been a lot of discussion about this feature and I'm sorry i haven't had time to contribute to the discussion.

I think this is a really important feature and I'd like to see it merged in the near future. But I'm hesitant to include this in the version 3 release. Here's my reasoning:

Implementing this requires bringing back the "dirtybox" behavior from the original Doom, which is a fragile and delicate mechanism to reintroduce. It's clear at this point that if anyone can do it right it's @nukeykt - but even then I kind of want to see this extensively tested first. Part of my hesitancy comes from the loading disk work, where we merged and then subsequently discovered there were visual bugs due to the way it had been implemented.

For me this seems like a risky change. I want to see sdl2-branch complete and a really solid v3 release, and then we can include this in v3.1, as it seems like a slightly more experimental feature.

But, this is a really important feature. I agree that it's a distinctive and memorable part of Vanilla Doom - even things like the phrase "hall of mirrors" are partly due to the shimmering effect which has been lost in modern source ports. This is great work and I'm glad to see it brought back.

[PorscheMonty]

Vanilla Doom was so buggy and quirky that source port developers would spend a considerable amount of time just ironing all this stuff out, making Doom "flat", predictable and more reliable, but this did not come without a price, the price of authenticity, which was eventually lost even to somewhat faithful source ports like prBoom and derivatives, yet if Chocolate Doom won't bring this back, a behavior that by nature lies at the very core of the scope of the project, then no one will.

3.0, 3.1, 3.2, branch it off into something else, it doesn't matter, just make sure the code laity will get to enjoy it.

+1111111111

[haleyjd]

What is the plan for special casing this for games that don't have it?

[nukeykt]

@haleyjd See mode_y variable. It is true for Doom and Strife and false for Heretic and Hexen. It enables triple buffering and dirtybox.

[fabiangreffrath]

Is it four? I tried recording in DOSBox and I only counted three images repeating in the HOM.

I am pretty sure there were three frame buffers.

Why? Have a look at the following code snippet from src/doom/d_main.c/D_Display():

     // change the view size if needed
    if (setsizeneeded)
    {
	R_ExecuteSetViewSize ();
	oldgamestate = -1;                      // force background redraw
	borderdrawcount = 3;
    }

[...]

   // see if the border needs to be updated to the screen
    if (gamestate == GS_LEVEL && !automapactive && scaledviewwidth != SCREENWIDTH)
    {
	if (menuactive || menuactivestate || !viewactivestate)
	    borderdrawcount = 3;
	if (borderdrawcount)
	{
	    R_DrawViewBorder ();    // erase old menu stuff
	    borderdrawcount--;
	}

    }

This means that each time the display size gets changed, the bezel is reconstructed and then drawn to the frame buffer in three successive tics (borderdrawcount) -- not four.

[linguica]

For me this seems like a risky change. I want to see sdl2-branch complete and a really solid v3 release, and then we can include this in v3.1, as it seems like a slightly more experimental feature.

Now that v3 is here, I hope we can finally get this feature implemented.