qbism / q2tools-220

Quake 2 map compiler tools with v220 map support, automatic phong, enhancements, and fixes.
GNU General Public License v2.0
61 stars 20 forks source link

Sky faces lit with garbage information #23

Open Paril opened 2 years ago

Paril commented 2 years ago

This is a legacy bug from the vanilla qrad3; SKY faces have early exits in a bunch of places. As a result, dface_t is left zero-initialized, leaving lightofs set to 0 and all styles set to 0. This causes all sky faces to just use whatever lightmap data happens to be at offset 0.

It seems like at some point post-release, id Software introduced this bug into their tools; most (if not all) vanilla maps (base1 as an example) have lightmap data for SKY. An easy example is base1, where the flyby ship that flies over sky receives proper lighting because it is always above a bright sky face.

Lighting base1.map from the tools in 4rad (or even qrad3) will cause all sky faces to use whatever lightmap happens to be at 0, causing a lot of flickering depending on the map.

Simple fix is to just allow sky to be lightmapped again. It doesn't need to be subdivided, because sky brushes are never in the surface cache in software mode afaik.

qbism commented 2 years ago

What issue does this cause? Is the sky lightmap data ever used? Sky faces are light emitters and never drawn AFAIK.

I guess SURF_WARP also garbage for same reason.

Edit: If there's a specific map with flickering I'll look at it.

Paril commented 2 years ago

Sorry for the late reply.

Yes, it is possible for entities to be on top of sky; see base1, the flyby in the start. It seems like the bug was introduced in qbsp3 after base1 was compiled, because the sky surfaces in base1 do have lightmap data. You can just recompile base1.map to see the issue; it sets lightofs to 0, causing all sky surfs to have garbage data in them.

qbism commented 2 years ago

Understood. Maybe this was removed to reduce lightmap size. Could some existing maps break limits if recompiled with lit skies?

Paril commented 2 years ago

Yeah I think it was done to reduce lightmap size too, but it seems Carmack left in a bug in the engine related to this; faces that have a lightofs of -1 are visually fullbright, but LightForPoint treats them as full-dark which is what causes this confusion. And of course, they left the lightofs of sky faces 0, so it pulls lightmap data from whatever face happens to be first in the map, instead of -1 for "no lightmap data" as it should be.

So far, nothing I've tried crosses any limits with sky lighting enabled. In theory you can put this behind an option since things being above a sky brush is pretty rare, but you should at least mark their lightofs as -1 and not 0, which is just garbage data relative to that face.

qbism commented 2 years ago

Would you mind doing a push request? I will add a toggle if you don't have one.

Paril commented 2 years ago

I'll see if I can find the time this week, I've been stretched thin lately heh