Novum / vkQuake

Vulkan Quake port based on QuakeSpasm
GNU General Public License v2.0
1.85k stars 224 forks source link

Bad water transparency layering in start level #656

Open Macil opened 1 year ago

Macil commented 1 year ago

In the start level (using the rerelease basedir), if you look at the water before episode 4, you can see that specific sections of the bottom surface incorrectly render over specific sections of the top surface. (Specifically, the upper surface's right third allows all of the lower surface to render over it, and the upper surface's left two-thirds allows the lower surface's far-left corner to render over it.)

vkquake0002

This isn't a recent issue. I've tested master and a few months-old commits and they each show the issue the same way. The issue is not affected by "r_alphasort 0".

I can consistently reproduce this on both MacOS and Linux that I've tested on.

Novum commented 1 year ago

@temx Not sure about this. Shouldn't the BSP recursive walk alpha sorting have fixed this one?

temx commented 1 year ago

That's not done for the worldmodel. It'd require walking the entire map tree looking for water surfaces, I haven't tried but I suspect it'd be quite slow for large maps. Maybe a pruned tree could be created at load time? I don't think it's worth it for one map with strangely split water.

temx commented 1 year ago

I did a quick test - see branch sorted-world-water, requires indirect path. It fixes this but slowdowns large maps by over 10x. R_RecursiveNode could be made faster, but it's unlikely this will ever be practical without some preprocessing.

temx commented 1 year ago

I added some basic pruning, the speed hit is greatly lessened but can still be in the >10% range.

sezero commented 1 year ago

The issue seems to happen in qs too: https://github.com/sezero/quakespasm/issues/53 .. but only in rerelease start map. Does this actually happen anywhere else?

Novum commented 1 year ago

@temx Okay, not really worth it. @sezero Seems like a new vis patch for the rerelease data would work?

sezero commented 1 year ago

@sezero Seems like a new vis patch for the rerelease data would work?

Really don't know, never ever tried it

temx commented 1 year ago

It's not a vis problem (though DOPA does have plenty of vis problems, since the VIS compiler used considers fence textures opaque - these can be fixed with r_novis 1 which is what the rerelease implicitly does).

The BSP tool chose to cut this water volume in an unfortunate order. I don't have visualization of the planes, but you can kind of guess where they are from the results: It sliced it vertically twice (into three parts) before the horizontal cuts, which would ideally be first. I don't think patching this in the BSP is practical.

I've made another tweak to the pruning, and now the cost for large levels is around 3-7% without tasks, and 1-2% with (it's usually not in the critical path). Probably not worth it for a single map.

Novum commented 1 year ago

Okay, I think I'll keep this bug open - perhaps forever.

sezero commented 1 year ago

So, it's a map-specific issue and not worth caring about much, yes?

Novum commented 1 year ago

Technically it's an engine bug, but there is cost to walking the BSP recursively back to front so it might not be worth doing it just for this case.

I believe the original Quake did a recursive walk for transparency but it was removed in later source ports for performance reasons.

temx commented 1 year ago

No transparency in the original, but it did walk front-to-back to get correct, zero overdraw drawing without a Z-buffer. Just don't ask how it handled brush or alias models (clipping the polygons into the world tree for the former, or building a Z-buffer just for the latter).

Novum commented 1 year ago

I actually know that! It was writing a Z value during zero overdraw span-based raster and alias was rendered with z check and write.

Macil commented 1 year ago

I found another place with bad water transparency layering: "Abyss of the troglodytes" (d2m8) of Dwell v2.2. Here's a save looking right at the issue: s2.sav.zip.

vkquake0000