ec- / Quake3e

Improved Quake III Arena engine
GNU General Public License v2.0
1.22k stars 153 forks source link

External lightmaps too bright since 2020-07-05 #80

Open Tibuq opened 3 years ago

Tibuq commented 3 years ago

Before and after:

i_view64_2021-01-05_01-17-44 i_view64_2021-01-05_02-05-28

Seems like only external lightmaps are affected. Above maps are dfwc2019-6 and dfwc2019-7

ec- commented 3 years ago

This was done on purpose to make color correction work as with built-in lightmaps

Tibuq commented 3 years ago

This was done on purpose to make color correction work as with built-in lightmaps

Are you saying, you made all maps using external lightmaps look way too bright on purpose? So now what, do we have to darken all external lightmaps of all maps and reupload them so they look right with the new q3e versions? And at the same time make them look way too dark in all other engines or older versions?

This seems like an awful idea, or am I missing something here?

Tibuq commented 3 years ago

The temporary workaround i found is to change the external lightmap paths (e.g. to /textures/lightmaps/) in the shader and move the lightmaps there. Now it behaves like a normal shader again, and everything looks like it used to.

I understand that you want to improve the way external lightmap work, and maybe it's the better way forward. But you have to consider all the maps that were created with the 'old' way, because they all look awful now in the recent q3e versions.

If this brigthening effect is desired, I think it should be implemented in a way that does not ruin all existing maps with external lightmaps. The 'external lightmap' hack was implemented a long time ago and is expected to work like it was implemented. If you want external lightmaps to work like internal lightmaps, you should implement that as a new alternative feature to the existing one, rather than just forcefully overwriting it.

ghost commented 3 years ago

Try r_overbrightbits 0

ensiform commented 3 years ago

Tbh the external lightmap 'hack' is just that, a hack. But iirc they can be loaded natively now?

Tibuq commented 3 years ago

Tbh the external lightmap 'hack' is just that, a hack. But iirc they can be loaded natively now?

Might be, I don't know. But either way, the point is, implementing new features and improving old features is great, but it has to be done in a way that doesnt screw with existing maps. And optimally also in a way that doesn't force me to release two versions of my map. One that looks right with new q3e versions and one that looks right with old q3e versions and other engines.

Wasn't that usually the philosophy when it comes to implementing new features like shaderx for example? When you use shaderx, you can still include a regular shader for older engines to read instead, so your map doesnt look bad in older engines. But with this new external lightmap brightening, you either have to avoid it by using the workaround described above, or you have to make your map exclusive to recent q3e engines and entirely disregard compatibility with older engines.

ensiform commented 3 years ago

Idd there, just pointing out

ec- commented 3 years ago

Are you saying, you made all maps using external lightmaps look way too bright on purpose?

Yes, it has been mentioned by @mightycow as a dedicated q3map2 feature used for new CPMA maps. Agrument as far as I understand - to avoid wasting time with _lmscale and other variables to get correct desirable result (i.e. save time for mappers).

How much maps were affected by that change?

ghost commented 3 years ago

I think this is related: https://www.quake3world.com/forum/viewtopic.php?f=10&t=54585, looking really awesome on Q3e!

Tibuq commented 3 years ago

Both these maps REQUIRE the CNQ3 1.52 exes to run, otherwise they are dark as hell. Its due to a new way that external lightmaps are being processed. If you wonder why its too dark, thats why!

Exactly my point. Now we have these amazing maps but they're not backwards compatible with older engines.

How much maps were affected by that change?

I don't know, someone would have to scrape through worldspawn and levelworld to find out. Not a bunch of mappers use external lightmaps, but just like the example above, the more advanced mappers are more likely to use external lightmaps, so I'm sure some great maps got affected by this.

Agrument as far as I understand - to avoid wasting time with _lmscale and other variables to get correct desirable result (i.e. save time for mappers).

Are you saying, the same results were already achieveable with lmscale? Yet you thought it was a good idea to implement this forced change and created this whole mess we have now?

Why not just change q3map2 to create the desired result without lmscale? This way, old maps are not affected, new maps are backwards compatible and you still achieve your goal.

This seems like such a dirty and messy 'fix' that really hasn't been thought through.

You might say it's too late to fix it now, but I still think you should. If you don't fix it, all old maps with ext lm have to be adjusted and re-released. If you do fix it, fKd has to recompile their 2 maps or adjust lightmaps and re-release. Both scenarios suck, but the more important point is that if you do fix it, fKd's maps and all future maps will be backwards compatible. I'm sure we can talk to Garux about implementing the q3map2 based method.

ghost commented 3 years ago

Yeah, probably it would be cool to have a 3rd renderer, like opengl_legacy or something, that supports old maps, and another one for fkd-ish maps. Somehow we always stumble over the same situation that maps don't look the same on all engines/renderers. This is the same for the new ioquake3 renderer, you can make really awesome maps, just to find out that the default renderer doesn't even support a minimum of the new features, etc. Or Wolfenstein - Enemy Territory, and it's TC mod/engine, for the later one all maps were compiled with map overbrightbits 0 in mind, they looked horrible in WET, and so on... This is just an idea, I don't want to argue against you, I just think/ask if multiple renderer could satisfy 'all' mappers.

Tibuq commented 3 years ago

My suggested solution (q3map2 method) already satisfies all mappers. A 3rd renderer wouldn't fix the mentioned issues.

ghost commented 3 years ago

Sounds plausible, probably the right time for you to get in contact with this Garux guy you mentioned.

Tibuq commented 3 years ago

only if q3e devs agree to revert this change. otherwise there is no point. we will also need their help to understand which compiler keys would give the desired result.

Garux commented 3 years ago

In the defrag maps repack ic 80 folders of external lightmaps (some of these are result of lightstyles hack to have flickering light, those appear to look fine with adjusted r_mapoverbightbits for some reason in vanilla engine 🤔 ).

Current fix for old maps + new q3e must be r_mapoverbightbits 1.

From the mappers perspective brightnes adjustment is dead simple: -brightness 2 (if no internal lms being in use along with external). This effectively does lm colour *= 2. Can't judge about whole range of technical consiquences here, but likely precision is even higher, than with engine doing <<1. Visually result looks equalish to internal lms with the default settings.

I recall having a couple of problems with external lms not being handled as internal, some of them, which i can recall r/n:

Blatantly it's hard to slap both halves of the ass appropriately here. I'd put it this way:

ghost commented 3 years ago

Probably it is worth noting that mapoverbrightbits > 0 still destroys the color range. So map overbrightbits should always be 0 in my opinion. Anyways, thats the point with overbrighting, people have different opinions whats right or wrong. Anyways, as long as people are working on very cheap monitors without calibrated colors it doesn't really matter.

ensiform commented 3 years ago

Isn't it default 1 in vanilla?

ghost commented 3 years ago

Iirc Q3: mapoverbrightbits 2; overbrightbits 1 Rtcw and WET changed their default values, as said, also when compiling stock maps. In Spearmint this was leading to a few problems (Spearmint is able to load nearly all idtech3 bsp versions). Zturtleman also did a few renderer corrections to adress this issue iirc.

ghost commented 3 years ago

Probably interesting (for developers): https://github.com/zturtleman/spearmint/wiki/Research:refEntity_t-Lighting-Control a good 'overall view' about various Q3 engines concerning refEntity_t (overbrightbits/models), because models are a seperate 'issue'. They aren't directly linked, so for example if you set r_mapoverbrightbits to 0 but keep your current r_overbrightbit value the game models aren't affected correctly, map models are affected depending on engine/map. I think (for ET) he introduced a new cvar for this (although it doesn't solve the general overbright issue).

ec- commented 3 years ago

Well, 80 folders with external lightmaps is too much so I'm reverting to old behavior for now

Tibuq commented 3 years ago

Well, 80 folders with external lightmaps is too much so I'm reverting to old behavior for now

Across lvlworld and worldspawn, i find 52 various maps in addition to 76 defrag maps using ext lm. So a total of 128 maps.

I'm glad you're reverting the change. Will my suggested q3map2 based solution be sufficient to achieve the desired result without altering the renderer?

Tibuq commented 3 years ago

Ok, after some testing I realize that my suggested solution probably doesn't make sense. But I came up with another solution that produces identical lighting with internal and external lightmaps, but it only works with q3 default overbrightbits 1 and mapoverbrightbits 2. Changing any of these settings creates differences between internal and external lm again.

Perhaps this isn't optimal, but maybe a good starting point to think about possible solutions or compromises. If it was up to me, noone would ever use anything other than obb1/mobb2 in q3 regardless, but maybe thats ignorant of me?

My solution was to adjust the q3map2 shader from:

{ // Q3Map2 defaulted
    {
        map maps/ext_lm_002/lm_0000.tga
        tcgen lightmap
        rgbGen identity
    }
    q3map_styleMarker
    {
        map textures/base_floor/concrete.tga
        blendFunc GL_DST_COLOR GL_ZERO
        rgbGen identity
    }
}

to:

{ // Q3Map2 defaulted
    {
        map maps/ext_lm_002/lm_0000.tga
        blendFunc add
        tcgen lightmap
        rgbGen identity
    }
    {
        map maps/ext_lm_002/lm_0000.tga
        blendFunc add
        tcgen lightmap
        rgbGen identity
    }
    q3map_styleMarker
    {
        map textures/base_floor/concrete.tga
        blendFunc GL_DST_COLOR GL_ZERO
        rgbGen identity
    }
}

obb1/mobb2 Photoshop_2021-01-08_03-21-33

obb0/mobb2 Photoshop_2021-01-08_03-27-51

Garux commented 3 years ago

Extra lightmap addition is pretty incorrect in the sense is light<->shadow ramp.

I thought a bit more about contras on suggested behavior change; one is ability to make map with hacked external lightmaps look equal with obb1 and 0. But since we've got fbo, this is not very relevant. So i guess suggestion if fair improvement, solving nice amount of inconveniences.

ghost commented 3 years ago

Before wasting resorces with incorrect shaders, I suggest just to edit your external lms (use your paint program). This is also a hack, but since you reverted back to default behaviour people should use their hacks as how they are used to.

Tibuq commented 3 years ago

What even is an 'incorrect shader'? If it achieves the desired result, it's good, no? I don't think you can get the same visual result by editing lm in paint program, can you? I just want you guys to find a solution to whatever it is that you wanted out of this change, that doesn't compromise backwards compatibility or affect old maps like it did.

Garux commented 3 years ago

One more issue with external lms hack is that they are affected by picmip. So either you force whole shader to nopicmip, or user will most likely get garbage lm appearence. Lm detection should be done via tcgen lightmap property, as lm tex coords are barely usable for anything else.

ensiform commented 3 years ago

I believe that they have a special loading flag to force them to be loaded at full size in Quake3e

Tibuq commented 3 years ago

lm tex coord are definitely usable and potentially useful for other things like masks.

Btw when's the next release planned? Would be good to avoid more mappers using the recent clients for their ext lm like fKd did.

Garux commented 3 years ago

lm tex coord are definitely usable and potentially useful for other things like masks.

That's why i say barely, issues of 0.1% of maps may be ignored, while solving issues of the rest.

Tibuq commented 3 years ago

if tcgen lightmap can be used for other things, there is no reason to make 'lm detection' (whatever that means) via tcgen lightmap when you can just make it via filepath maps/mapname/ or via image name lm_xxxx.

What do we even need 'lm detection' for though?

Garux commented 3 years ago

make it via filepath maps/mapname/ or via image name lm_xxxx.

this way is not robust, as such naming convention is exclusively of q3map2 may also add blendfunc filter property to check list to be more concrete

What do we even need 'lm detection' for though?

for adressing listed issues

ec- commented 3 years ago

It is possible to use some unique q3map_* key for that purpose

Garux commented 3 years ago

It is possible to use some unique q3map_* key for that purpose

Is even less persistent hackery, as there are no such 100% mandatory keys; also they may be removed before shader release, as unneeded for engine (e.g. i did this for shaders in maps repack).

Tibuq commented 3 years ago

am i guessing correctly that you want to find a way to detect lightmaps in new maps and apply the brightening to them, while keeping old maps unbrightened? or what's the plan here? im a bit lost.

ec- commented 3 years ago

It may be possible to use handwritten key for such maps

ioridyson commented 3 years ago

Hello, author of cpm24a_fkd and bones_fkd here, I've pulled the maps and I'm going to recompile them using the standard light value of brightness 2 in the shaders to maintain backwards compatibility as I think at this point it's silly to limit the amount of engines the user can play the maps on. But tbh I'm busy with a mapping competition right now so its gonna be done later :)

luisvalenzuelar commented 3 years ago

Well, 80 folders with external lightmaps is too much so I'm reverting to old behavior for now

Sorry to carry on with this but is the latest (reversed?) version free of this problem? I'm still seeing very bright lights, particularly with explosions effects. Unless that is a local config unrelated to this...