fabiangreffrath / crispy-doom

Crispy Doom is a limit-removing enhanced-resolution Doom source port based on Chocolate Doom.
https://fabiangreffrath.github.io/crispy-homepage
GNU General Public License v2.0
807 stars 132 forks source link

Rise sprites limits #14

Closed ghost closed 10 years ago

ghost commented 10 years ago

Monsters in NUTS.WAD still flickers due to a limit regarding number of sprites showing on the screen simultaneously. Maybe the limits should be risen a bit?

http://www.doomworld.com/idgames/index.php?file=levels/doom2/Ports/m-o/nuts.zip

fabiangreffrath commented 10 years ago

Wow, even more MAXVISSPRITES? They have already been rised by factor 8 and are now at 1024. Could you estimate if another factor 2 or 4 would solve the issue?

plumsinus commented 10 years ago

NUTS is a pretty extreme example though, there aren't too many other maps that have that many things but still low linedef counts. I'm not saying "don't do it" but if you were going to raise limits of anything I think more visplanes and drawsegs would be more beneficial overall.

Anyhow another *4 would probably be good enough for NUTS, there'd still be some flickering but I think it wouldn't be too bad.

plumsinus commented 10 years ago

NUTS.WAD also crashes with a Z_Malloc error when you exit the level, I don't know whether that changes with MAXVISSPRITES or not. http://doomwiki.org/wiki/Z_Malloc_error Basically, I don't think that NUTS is a good benchmark wad.

fabiangreffrath commented 10 years ago

Indeed, and that's why I am hesitating to rise the limits again. What if I get another bug report tomorrow that Crispy Doom has issues with DOUBLENUTS.WAD that shows off twice as many monsters?

fabiangreffrath commented 10 years ago

NUTS.WAD also crashes with a Z_Malloc error when you exit the level, I don't know whether that changes with MAXVISSPRITES or not.

That crash occurs because the wipe effect doesn't find enough memory to allocate for buffering of the screen. It can be fixed by raising MIN_RAM and DEFAULT_RAM in i_system.c a bit. Maybe I should do this for the next release.

plumsinus commented 10 years ago

Hm, does this mean it's worth raising MAXVISSPRITES and/or limits some more? Or are you still inclined to keep them as they are?

It would be nice to play NOVA under Crispy Doom. (Apart from the levels with too large a BLOCKMAP, which is rather a different issue - though perhaps the maps could be tweaked slightly to get the blockmap down, if that were the only problem.)

fabiangreffrath commented 10 years ago

Theoretically, yes. Is only MAXVISSPRITES affected?

fabiangreffrath commented 10 years ago

In other words, which limits need to get raised again in order to play NOVA? Only MAXVISSPRITES? Or also MAXVISPLANES and MAXDRAWSEGS? Or what else?

bradharding commented 10 years ago

I've used NOVA.WAD as a test case in developing DOOM RETRO. The limits that need to be raised depend on the map. It's not just MAXVISSPRITES. To get MAP24 working, the blockmap limit needs to be removed (see 7551ec322c20e568859b82a95325649094e7a8dd), and for MAP30, extended nodes need to be supported (see 97a1fda97b09c513c7bfcb1bc20460804a66ed67). Also, a part of MAP19 crashes because of the visplane limit (see 8323e8a53459153ae53449c3f5bf2a32e3abbfda). And then also the save game limit needs to be raised or removed.

plumsinus commented 10 years ago

Sorry, my previous comment was a bad one - there are two separate issues there, only connected by increase of some limits. And then I was away for a few days!

For NUTS.WAD: the only thing to get it working properly is more MAXVISSPRITES. Another * 4 would probably suffice. While I agree that there has to be a limit somewhere, and that the current limit will suffice for most things, NUTS.WAD is somewhat of a "historic" wad and if it will no longer crash due from Z_Malloc errors, it is (maybe) worth considering adding improved support for it. Perhaps even another * 2 would make it "playable", as much as that level is playable anyhow.

For NOVA.WAD:

bradharding: your links don't work, I assume they're meant to point to Doom Retro commits?

plumsinus commented 10 years ago

Links to Doom Retro commits:

blockmap limit removed: https://github.com/bradharding/doomretro/commit/7551ec3

extended nodes support: https://github.com/bradharding/doomretro/commit/97a1fda

visplane limit: https://github.com/bradharding/doomretro/commit/8323e8a

bradharding commented 10 years ago

Whoops! Sorry about the links! Note to self: don't ever assume Github's markdown is clever enough.

fabiangreffrath commented 10 years ago

Thank both of you for the helpful pointers!

For NUTS.WAD: the only thing to get it working properly is more MAXVISSPRITES. Another * 4 would probably suffice.

Agreed, the next release will have an additional *4 for MAXVISSPRITES. But I'd rather not increase it any more than that.

a few maps crash due to visplane limits (and only that): 19, 25, 28, and possibly others. These are the ones I'd like to see fixed, since the fix is fairly easy

Do you believe another *4 for MAXVISPLANES will suffice? I am reluctant to implementing dynamic visplanes the way Boom (and Doom Retro) do, because I consider this a bit too intrusive in the "engine guts" and I'd like Crispy to remain as close to Choco as possible in this regard.

maps 24, 30, and 31 have BLOCKMAPs too large for vanilla (and chocolate/crispy)

Removing the blockmap limit is another intrusive non-trivial change that I'd rather like to avoid.

Honestly, I think a map that boasts BLOCKMAP (24, 30, 31) or requires extended nodes (30) is just too vanilla-incompatible for Choco or Crispy. I'd recommend playing these in PrBoom+ or another more advanced source port rather than fixing Crispy Doom around them.

fabiangreffrath commented 10 years ago

Removing the blockmap limit is another intrusive non-trivial change that I'd rather like to avoid.

Well, not that intrusive, actually. Will be part of the next release.

Still not sure about the visplanes limit, though.

plumsinus commented 10 years ago

I think another *4 for both limits is a good maximum, I agree that going past that is getting too vanilla incompatible. Hopefully that will make both maps playable, but if not, then I'd probably consider them too complex for Crispy Doom.

If you're increasing those limits, you may also want to raise MAXDRAWSEGS as well, to avoid visual glitches. All the other limits should be fine as they are.

fabiangreffrath commented 10 years ago

Alright, so the next version will have *4 for MAXVISSPRITES, MAXVISPLANES and MAXDRAWSEGS. It will also have the BLOCKMAP limit removed. However, there is still an integer overflow in the automap when zooming out of huge maps (e.g. arcadia) that I have not yet wrapped my head around.

Brad, your BLOCKMAP code seems to come from Doom Legacy. Have you ever had a look at the Boom implementation? It seems a lot easier to understand to me.

fabiangreffrath commented 10 years ago

Short status update regarding NOVA: Maps 19, 25, 28 seem to play fine now. Map 24 crashes in R_ClipSolidWallSegment() and I still have no idea why. Escpecially since it runs in WinMBF where I got P_LoadBlockMap() from. Any hint here is highly appreciated! Maps 30 and 31 do also immediately crash, but they do also crash WinMBF even if rebuilding BLOCKMAP is enforced by passing "-blockmap".

fabiangreffrath commented 10 years ago

Map 24 crashes in R_ClipSolidWallSegment() and I still have no idea why.

Because there is also a MAXSEGS limit that wants to get raised. Now everything works except for Maps 30 and 31. \o/

fabiangreffrath commented 10 years ago

Another status update: Now that support for extended nodes (yep!) got implemented, the BLOCKMAP limit removed and the other static limits raised again, all maps in NOVA are playable -- except for a severy case of Medusa in map29.

plumsinus commented 10 years ago

Wow, that's pretty exciting. Extended nodes aren't too intrusive a change?

BTW I just remembered there's one NOVA map that crashes because it exceeds MAX_ADJOINING_SECTORS. Is that an easy limit to bump up?

fabiangreffrath commented 10 years ago

Extended nodes fortunately only affect internal data structures and I could use code from PrBoom+.

The crashing NOVA map is #16. MAX_ADJOINING_SECTORS is a simple #define that's currently set to 20. Doom95 apparently raised this to 500, but there is a lot of code around there to properly emulate overflows in both Choco and PrBoom+, so I am not sure if I should just change that. On the other hand, maps that overflow this are perhaps so broken that we shouldn't even expect them to keep demos in sync, anyway. Let's just dare...

plumsinus commented 10 years ago

You are correct, it was MAP16 I was thinking of.

I agree with demos probably being inherently broken for maps that need this limit removed. However, if you want a test case, here's a skill 1 speedrun of MAP16 done in PrBoom+ with shorttics that exits in about 2 minutes. Plays back fine in current Crispy Doom until the problematic switch. No comments on my poor speedrunning skills please ;)

https://www.mediafire.com/?kxbg2738jgegq83

fabiangreffrath commented 10 years ago

With my latest additions to GIT the demo finishes in Crispy Doom without a flaw.

plumsinus commented 10 years ago

Regarding your latest commit, I think you are correct that the Doom95 adjoining sectors limit is 50. I did a quick test with doom95.exe (ugh) and after 50 sectors, the linedef fails to operate. (Doesn't seem to crash like vanilla though.)

If you want, here is the wad I used: https://www.mediafire.com/?it3v3walbq17473

fabiangreffrath commented 10 years ago

With the latest commit, the limit is now dynamically increased as needed. Thanks for the map anyway, your testing is highly appreciated!

fabiangreffrath commented 10 years ago

Plums, can you confirm that this version fixes all the issues with NUTS.WAD and NOVA.WAD that we discussed here so far?

www.greffrath.com/~fabian/crispy-doom_1.4+a2e19a73.exe

plumsinus commented 10 years ago

First of all, I have now successfully built crispy doom on my machine, so I will be able to test development versions easily from now on. There are still a few issues, mainly that the SDL dlls I get from compiling give me laggy sound, fixed by copying the ones from the latest release instead. I tested using your build above as well, just to make sure any problems were not from me compiling.

Secondly, the automap doesn't work at all in the current version.

NUTS.WAD seems to work fine as far as visible sprites and things go, and doesn't crash with a Z_Malloc error. There is still a rendering issue that was also present in previous crispy doom versions (that I had forgotten about or not noticed, perhaps) where a segment of the wall to the right doesn't display properly. The left side, oddly, doesn't have this problem.

doom00

NOVA.WAD mostly works, but with a few problems still:

MAP30 has several sections where textures don't display properly, either coming up as black or just transparent (probably depending on whether they're 2s or not). You can test this by starting the map, hitting the switch, moving to the right FIREBLU portal that is visible, and then opening the door - many textures in the next area are not displayed. There is also an issue with black columns in the all-sky area behind the Baphomet face - this may be related, or may even be a mapping error, I haven't looked in an editor yet.

Beyond that, there is one buggy level that crashes, which I am about to file another issue about, and the music for MAP25 doesn't play because it exceeds the filesize for MIDIs - in general a problem I'm not bothered about at all.

Thanks for all your work on supporting these maps!

fabiangreffrath commented 10 years ago

First of all, I have now successfully built crispy doom on my machine, so I will be able to test development versions easily from now on.

That's very good news!

Secondly, the automap doesn't work at all in the current version.

Damn! I have changed some variable types to be able to zoom far out in huge maps. Seems like Windows doesn't cope too well with it...

a segment of the wall to the right doesn't display properly.

Indeed, I can see this as well. However, this glitch is also present in MBF, but not in PrBoom+. Not sure what might be the cause for this, though.

many textures in the next area are not displayed. There is also an issue with black columns in the all-sky area behind the Baphomet face

Hm, not sure what you mean. could you send a screenshot and/or a savegame?

there is one buggy level that crashes, which I am about to file another issue about

If that's that "unknown special sectors" thing, that's already fixed. :)

music for MAP25 doesn't play because it exceeds the filesize for MIDIs

Ah, strange. Is there any engine limit these maps don't hit?

plumsinus commented 10 years ago

Missing textures on NOVA MAP30: doom01 doom07

Black columns in the sky - looks like the same bug, probably. You can get to this area by using IDCLIP at the start of the map and then just walking forward for a while: doom04 doom05

The unknown specials error is the problem on that other map (MAP27). Nice to hear it's fixed already.

As for nuts.wad: DOS Boom.exe also shows that rendering error. I added a vertex and rebuilt the nodes for the map, and it goes away. Perhaps a giant slime trail, or some other mapping error, but whatever the case probably not something you need to try to fix.

fabiangreffrath commented 10 years ago

music for MAP25 doesn't play because it exceeds the filesize for MIDIs

There was a hard-coded file size limit (90kB) in Choco that I just removed.

Missing textures on NOVA MAP30

These were the notorious "linedef XY has two-sided flag set, but no second sidedef" warnings from PrBoom+. Fixed with the fix found there.

Black columns in the sky

These are HOMs, apparently. They also show in MBF, but not in PrBoom+. No idea what might fix them.

So we are left with:

plumsinus commented 10 years ago

Yes, I think those 3 issues are all that is left.

segment rendering glitch in NUTS

It looks like a mapping/nodebuilder error. Does PrBoom+ rebuild nodes ever?

sky HOMs in NOVA/30

If it's HOM and not related to the other problem, then also maybe a mapping error? IIRC to get sky textures on both floor and ceiling you need some trickery. But then if PrBoom+ displays it correctly, I don't know. I guess the incorrect 2s flags are also mapping errors technically.

There was a hard-coded file size limit (90kB) in Choco that I just removed.

hah, figures. :)

plumsinus commented 10 years ago

sky HOMs in NOVA/30

This is indeed a mapping error, at least for vanilla type mapping. The proper way to do this would be to surround the sector with F_SKY1 on both floor and ceiling with a sector that has F_SKY1 on the floor only and a ceiling height equal to the main sky sector, and then another sector with a sector height of 1. Hell Revealed (hr.wad) MAP26 does this properly.

It would still be worth fixing if you can find the solution, since I'm sure many other maps do it the "wrong" way if PrBoom+ displays it correctly this way.

fabiangreffrath commented 10 years ago

It looks like a mapping/nodebuilder error. Does PrBoom+ rebuild nodes ever?

PrBoom+ has R_PointToAngleEx(). If I use that in Crispy, the giant slime trail goes away. But it's a costy calculation...

plumsinus commented 10 years ago

Well my personal opinion is that it's not the job of the source port to fix all mapping errors, especially if there's a big CPU cost to fixing them, and so you should leave it as is. Up to you though. (yes I know this statement contradicts my last post a little ;P)

fabiangreffrath commented 10 years ago

It would still be worth fixing if you can find the solution, since I'm sure many other maps do it the "wrong" way if PrBoom+ displays it correctly this way.

if ceilingplane and floorplane were the same (e.g. the sky) the same visplane would get used for both, resulting in the HOM. I have added a simple check for this case.

fabiangreffrath commented 10 years ago

Plums, could you please check out the latest revision from GIT and confirm that really all the issues we discussed in this thread (affecting both nuts.wad and nova.wad and possibly others as well) are fixed now? If you encounter any other issues, please do not hesitate to report them... I have just picked up steam. :)

plumsinus commented 10 years ago

Confirmed that nuts.wad and nova.wad play without errors*. The automap is back too. However performance in nuts.wad is considerably worse for me than with crispy-doom_1.4+a2e19a73.exe, likely on account of having no limit of visible sprites anymore. (Performance is fine with -nomonsters, or when the automap is open.) I do have a rather old computer, so I leave it up to you as to whether that's a problem -- I do not plan on playing nuts.wad for non-testing purposes any time soon.

* There is also another visual bug I noticed just now with nuts.wad: as you walk towards a corner, the wall textures seem to gradually grow in size and become non-level, then shrink back down. This effect seems to be present in any software rendered port, but I notice it more in Crispy Doom, maybe just on account of the lower resolution? Again, not a problem I'm too concerned with personally, but you did just say "do not hesitate to report them"... ;)

doom00 doom01

fabiangreffrath commented 10 years ago

However performance in nuts.wad is considerably worse for me

Given that I have just made it to display more than 9000 [sic!] sprites in nuts.wad, which is plain "nuts" even for nuts.wad, I think it is safe to cap the sprite limit at 128*32=4096, i.e. the extended raised limit that I originally had planned and that applied in the previous revision that you tested. That is, the array will be extended up to 4096 sprites and then it will behave as a static array similar to the one in Vanilla.

There is also another visual bug I noticed just now with nuts.wad

Wow, now I cannot "unsee" this anymore! What could be the cause for this?

bradharding commented 10 years ago

Would it be this, perhaps: http://doomwiki.org/wiki/Long_wall_error?

plumsinus commented 10 years ago

That is certainly the error, bradharding. If I divide the walls in nuts.wad into 1024-long segments, the effect is greatly diminished.

fabiangreffrath commented 10 years ago

I think it is safe to cap the sprite limit at 32*MAXVISSPRITES

This is commited now. Could you test if the performance is back?

http://doomwiki.org/wiki/Long_wall_error?

Is there any known cure to this effect?

plumsinus commented 10 years ago

Performance is back to the level it was at before - still a bit choppy for me, but not a slideshow. Even with 4096 sprites there is still some flickering in the corners of the map, but I'd say this is a good compromise.

There's no easy way to make sprites closer to the player get drawing priority, is there?

Is there any known cure to this effect?

Seeing as doomwiki says "In extreme cases (walls of greater than 10000+ units in length), collision detection stops, and things are able to pass right through a wall.", I would imagine not without breaking vanilla behaviour/demo compatibility.

I just tested PrBoom+ in software mode at 640x480 and it looks very similar (i.e. equally as severe) as in Crispy Doom.

fabiangreffrath commented 10 years ago

There's no easy way to make sprites closer to the player get drawing priority, is there?

I don't think so. As it seems, the vissprites array is filled per subsector until the static limit is reached. When all subsectors have been processed, the list of vissprites is ordered by sprite scale. Sprites are ordered from back to front, so that closer sprites are drawn later and cover those which are further away. Sprites in subsectors which were processed when the vissprites array was already filled are not processed at all. :(

So this procedure would have to get changed into

which is just about as slow as drawing them all in the first place. :/

fabiangreffrath commented 10 years ago

I've tried it. You could post-process the sorted vissprites list like this (add at the end of R_SortVisSprites) to show only the 20 closest vissprites. However, the performance gain is zero, if not even counter productive.

for (i = 0 ; i < count - 20; i++) { vsprsortedhead.next = vsprsortedhead.next->next; }

fabiangreffrath commented 10 years ago

Update: Even entirely suppressing sprite rendering by commenting out R_DrawSprite() in R_DrawMasked() does not help gain performance. i am giving up on this.

plumsinus commented 10 years ago

Thanks for giving it a shot anyhow.

fabiangreffrath commented 10 years ago

I am quite confident with the current state in GIT. Since I'll be on vacation until next weekend, I'd apreciate if you could test the current snapshot a bit and report any issues, so I can fix them when I'm back. I'd like to release what we have in GIT next week.

plumsinus commented 10 years ago

Sure. You may have seen on the DW forums that I'm doing bugfixing/testing for NOVA, for a 1.1 release. My plan is to test both Crispy Doom and NOVA with each other. Hopefully neither will have issues.

Weapon recoil is an interesting addition.

Have a nice time!

fabiangreffrath commented 10 years ago

My plan is to test both Crispy Doom and NOVA with each other.

Sounds like a good stress test for both.

Weapon recoil is an interesting addition.

I though so, and it was trivial to implement. If you have any further feature suggestions, please bring them up.

Thank you very much already!