[Carmack] Sprites + Wrapped Midtextures + 3DFloors = Holes

Forum rules
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.

Post a reply

Smilies
:D :) :( :o :shock: :? 8-) :lol: :x :P :oops: :cry: :evil: :twisted: :roll: :wink: :geek: :ugeek: :!: :?: :idea: :arrow: :| :mrgreen: :3: :wub: >:( :blergh:
View more smilies

BBCode is OFF
Smilies are ON

Topic review
   

Expand view Topic review: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Holes

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by drfrag » Fri Mar 09, 2018 11:52 am

Rachael wrote:Those are broken 1-col drawers. They were fixed in a later commit.
Thanks very much, i guess mystery solved then. If only i could port the fix from LLVM to C++... First i'll need to find out which drawer i'd need to fix. :oops:
dpJudas wrote:Fixed the midtexture clipping error in MAP08 of Unloved2.
Well done! :)

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by dpJudas » Thu Mar 08, 2018 6:32 pm

Fixed the midtexture clipping error in MAP08 of Unloved2.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Rachael » Thu Mar 08, 2018 2:49 pm

Well it looks a lot better, but I can see the original issue Randi was trying to address. (That didn't actually get addressed with her fix)

In MAP08 of Unloved2 in the hospital, if 3D floors are present, midtextures can no longer clip at all, they are always drawn as if clipping is turned off. The compat options are already set such that midtextures should always clip, and this was done because the mapset was originally for the OpenGL renderer, before we had 3D floors and truecolor and stuff.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Xaser » Thu Mar 08, 2018 12:54 pm

Thanks! Just made a fresh build from master and everything's A-OK here. Good to see the code infinitely less janky now too.

I'll keep an eye out for any stray midtex phantoms in the future, just for pseudoscience's sake, but I doubt there's anything to be found.

[EDIT] This can be closed as [Fixed] now.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by dpJudas » Wed Mar 07, 2018 6:34 pm

OK, I've decided that what Randi was doing made no sense whatsoever. I don't know what bug she was trying to fix, but the fix was certainly incorrect. I effectively committed your change to master, but I didn't do it via your PR as I took the opportunity to move the backup copy code closer to its correct location.

Now on to eradicate the fake3D completely - probably the stupidiest variable in all of the swrenderer. It packs two enums into one integer and then also uses bits for booleans for the goal of saving 36 bytes of system memory..

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Xaser » Wed Mar 07, 2018 5:48 pm

dpJudas wrote:bkup is always a copy of the original ceilingclip and never changes.

...

Randi's fix basically says that #3 should not begin from fresh if it is a wrapped midtex. Why would that ever be needed? At what height should a wrapped midtex end? The ceiling?
Yeah, the more I noodle this, the less it makes sense. If sprtopclip is restored from bkup, then the real ceiling clipping should be correct. Here's the place in RenderDrawSegment::RenderWall that pulls mceilingclip from sprtopclip.

If Randi instead meant that midtextures were bleeding into the ceilings of 3D floors, then you'd think that the culprit would live here. Does anything seem awry here versus the non-wrapped-midtex version of the same code?

It feels more and more like we're chasing a ghost.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Rachael » Wed Mar 07, 2018 4:36 pm

I wonder if it might be one of the many things that never actually got finished in the renderer before and during the floating point rewrite.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by dpJudas » Wed Mar 07, 2018 4:34 pm

I took a look at the code and analyzed it a bit further.

bkup is always a copy of the original ceilingclip and never changes.

RenderDrawSegment::Render itself gets called in two situations: in RenderTranslucentPass::DrawMaskedSingle and VisibleSprite::Render.

The visible sprite render is called first and here fake3D does not have the FAKE3D_REFRESHCLIP flag yet. The flow is thus as follows:

1) Calls RenderDrawSegment::Render for each draw segment behind a sprite. Draws the segment columns behind the sprite. Fills those ranges in 'sprtopclip' with 'viewheight'.
2) Calls RenderDrawSegment::Render for all draw segments. This draws the full segment, except for those ranges cleared in step #1.
3) At the end of the second RenderDrawSegment::Render call it copies back the backup. This ensures the next call to DrawMaskedSingle begins from fresh.

Randi's fix basically says that #3 should not begin from fresh if it is a wrapped midtex. Why would that ever be needed? At what height should a wrapped midtex end? The ceiling?

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Graf Zahl » Wed Mar 07, 2018 4:02 pm

Hi-Tech Hell is also a great thing to test 3D floors. Since it's made for Doom Legacy it doesn't have any slopes that may cause problems.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Rachael » Wed Mar 07, 2018 3:59 pm

A really good map set to test 3D floors and sprites/midtex's with is Unloved and Unloved 2. The extensive 3D floor usage in these mod makes it an ideal candidate for bug troubleshooting with the software 3D floor system, as the problems occur in here in droves. It's been my experience that no matter what I do to fix it, something else breaks.

Make sure to run 'git blame' over the code, as well, because there may be modifications from me in that code from about a year or so ago. You'll likely want to revert those commits.

A couple good places:

Unloved - MAP01 (-246, 372, 67)
Unloved 2 - MAP08 (48, 309, -1334)

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Xaser » Wed Mar 07, 2018 3:54 pm

Thanks for the breakdown! That helps a bit -- it's still quite arcane, but it at least identifies a few troublesome spots.
dpJudas wrote:If [a draw segment is behind a sprite], then it draws only the part of the draw segment covered by the sprite.
The obvious bit, I suppose: this is pretty clearly the step that's getting "skipped." The "if(!wrap)" check is unconditionally disabling the bkup restore, meaning none of these behind-sprite draws happen at all since sprtopclip's been trashed. This part makes sense.

The mystery, then, is trying to figure out exactly what case Randi found in which restoring from bkup was a bad idea. We really don't have anything to go off of aside from the commit description ("Tiled midtextures could go up past the ceiling when a 3D floor is in view"), but judging from that alone, here's an educated guess of sorts:

The big scary 3D floor pass in RenderTranslucentPass starts from the top of the screen, iterates through all 3D floors from top to bottom, and re-renders the scene until it hits the player's viewz. At this point, it flip-flops and starts rendering the remaining 3D floors from the bottom up. Seems like a setup for a classic off-by-one error: either on the first, last, or "middle" pass (the one where it switches from top-down to bottom-up), sprtopclip gets filled with [something bad] from bkup that causes the drawsegment to render infinitely upward. Presuming that's what Randi meant in that commit desc, of course. ;)

This is all pure conjecture unless we can actually trigger Randi's bug though. I'll muck about with Doom Builder soonish to see if I can produce anything that matches the description.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by dpJudas » Wed Mar 07, 2018 2:54 pm

Xaser wrote:What can I do to help get this merged before release? I can try and play around with trying to reproducing whatever issue Randi fixed, though it'll be a bit guessworky without some assistance.
I'm not sure how much this will help, but I'll try explain what the code is doing and why (as far as I understand it):

Initially the renderer walks the BSP tree and draws all solid walls. As it does this it keeps track on what has already been drawn vertically as min/max values for each pixel column on the screen. Essentially this is the top of the mid wall stored in 'ceilingclip' arrays and the bottom in 'floorclip' arrays. For each two-sided midtexture it creates a DrawSegment for the wall, but it does not draw it yet. As part of this creation it copies the clip arrays into 'sprtopclip' and 'sprbottomclip' on the DrawSegment.

Once the renderer is done drawing all the solid walls and planes, it draws the sprites. The sprites are drawn back to front, but before each sprite is drawn it checks if a draw segment is behind the sprite. If it is, then it draws only the part of the draw segment covered by the sprite. It then marks that part of the draw segment drawn by filling 'sprtopclip' with values beyond any 'sprbottomclip' value, thus effectively causing everything in that range to be fully clipped. That's what the 'fillshort' lines are about.

After it is done drawing all the sprites it iterates through all the draw segments again and draws them all back to front. All the places that were covered by sprites have the clip arrays modifies so they skip those blocks so sprites don't get overwritten in this last draw call.

This is Carmack's original (brain dead) way of sorting segments and sprites. On top of this 3D floors were added. I'm not 100% sure of the attempted algorithm here, but for each 3D floor height it draws only sprites above or below that floor level. Since it reuses the original sprite drawing loops, it runs into the problem with sprtopclip being cleared as part of the drawing as it now calls it multiple times. That's where the 'bkup' array enters the picture. The code attempts to take a backup of the array, so that other code can mess up the array and then it can copy back the original. The code you changed made it always copy back the backup. Apparently Randi identified a situation where restoring the array wasn't good for 3D floors.

That's about as far as I can help. The exact details of 3D floor segment/sprite clipping eludes me and for softpoly I opted for a much easier way: split/sort sprites by their subsector before drawing them. That way you have the BSP guaranteeing that wall segment is in front all sprites drawn so far and behind the rest.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Graf Zahl » Wed Mar 07, 2018 2:04 pm

Xaser wrote:What can I do to help get this merged before release?
This won't get merged before a release, unless someone can prove beyond reasonable doubt that the issue the reverted commit was supposed to fix wasn't worth fixing and this change doesn't break anything.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Xaser » Wed Mar 07, 2018 1:52 pm

What can I do to help get this merged before release? I can try and play around with trying to reproducing whatever issue Randi fixed, though it'll be a bit guessworky without some assistance.

Re: [Carmack] Sprites + Wrapped Midtextures + 3DFloors = Hol

by Rachael » Wed Mar 07, 2018 1:20 pm

Those are broken 1-col drawers. They were fixed in a later commit.

Top