support BRGHTMPS lump from Doom Retro #846

rfomin · 2022-12-16T15:55:18Z

Tested on this lump: https://github.com/bradharding/doomretro/blob/master/res/BRGHTMPS

I used u_scanner.c but I think we could use it in Crispy too? (potential support for UMAPINFO).

src/r_bmaps.c

fabiangreffrath · 2022-12-16T16:36:20Z

Thanks for starting the work on this! I wonder if we should parametrize our internal default values in a similar way?

rfomin · 2022-12-16T16:41:37Z

Thanks for starting the work on this! I wonder if we should parametrize our internal default values in a similar way?

Add BRGHTMPS lump to autoload folder? Or do you mean reorganize the arrays in r_bmaps.c?

fabiangreffrath · 2022-12-16T16:51:19Z

I'm not convinced of using the autoload folder for such features. Think of all the bfgdoom2.wad and freedoom2.wad and miniwad.wad and doom2_19.wad and udoom.wad etc IWAD files around.

I think we should handle it as we do for colored blood. Provide an internal default and if a PWAD provides its own, use this without asking.

rfomin · 2022-12-16T16:59:33Z

Think of all the bfgdoom2.wad and freedoom2.wad and miniwad.wad and doom2_19.wad and udoom.wad etc IWAD files around.

That's why we have doom-all

Provide an internal default and if a PWAD provides its own, use this without asking.

This is how it works in the current PR. Maybe we should somehow reorganize the data in r_bmaps.c.

fabiangreffrath · 2022-12-16T17:09:33Z

That's why we have doom-all

Hm, true.

Maybe we should somehow reorganize the data in r_bmaps.c.

That's exactly what I wanted to suggest in the first place. 😁

fabiangreffrath · 2022-12-17T08:11:58Z

This is how it works in the current PR.

You can still disable the feature even if the lump is provided. I think this is handled different for blood color and thing translucency.

Maybe we should somehow reorganize the data in r_bmaps.c.

Not sure if this works for anything else than texture brightmaps, though.

fabiangreffrath · 2022-12-17T13:28:24Z

Could you try this?

diff --git a/src/m_menu.c b/src/m_menu.c
index cd203da..19c5ec1 100644
--- a/src/m_menu.c
+++ b/src/m_menu.c
@@ -4030,12 +4030,13 @@ enum {
 static void M_UpdateStrictModeItems(void)
 {
   extern boolean deh_set_blood_color;
+  boolean wad_set_brightmaps = (W_CheckNumForName("BRGHTMPS") >= 0);
   // map_player_coords
   DISABLE_STRICT(auto_settings1[auto1_coords]);
   DISABLE_ITEM(strictmode || deh_set_blood_color, enem_settings1[enem1_colored_blood]);
   DISABLE_STRICT(enem_settings1[enem1_flipcorpses]);
   DISABLE_STRICT(gen_settings4[gen4_realtic]);
-  DISABLE_STRICT(gen_settings2[gen2_brightmaps]);
+  DISABLE_ITEM(strictmode || wad_set_brightmaps, gen_settings2[gen2_brightmaps]);
   DISABLE_STRICT(gen_settings3[gen3_level_brightness]);
   DISABLE_ITEM(strictmode && demo_compatibility, gen_settings1[gen1_trans]);
   DISABLE_STRICT(gen_settings3[gen3_palette_changes]);
diff --git a/src/m_misc.c b/src/m_misc.c
index 7527c1e..c9f678d 100644
--- a/src/m_misc.c
+++ b/src/m_misc.c
@@ -106,7 +106,7 @@ extern int opl_gain;
 extern int midi_player_menu;
 extern boolean demobar;
 extern boolean smoothlight;
-extern boolean brightmaps;
+extern boolean default_brightmaps;
 extern boolean r_swirl;
 extern int death_use_action;
 extern boolean palette_changes;
@@ -260,7 +260,7 @@ default_t defaults[] = {
 
   {
     "brightmaps",
-    (config_t *) &brightmaps, NULL,
+    (config_t *) &default_brightmaps, NULL,
     {0}, {0,1}, number, ss_gen, wad_yes,
     "1 to enable brightmaps for textures and sprites"
   },
diff --git a/src/r_bmaps.c b/src/r_bmaps.c
index cfd234c..76ff7b1 100644
--- a/src/r_bmaps.c
+++ b/src/r_bmaps.c
@@ -26,7 +26,7 @@
 #include "m_misc2.h"
 #include "u_scanner.h"
 
-int brightmaps;
+int brightmaps, default_brightmaps;
 
 // [crispy] brightmap data
 
@@ -1093,6 +1093,9 @@ static const byte *R_BrightmapForTexName_Lump(const char *texname)
 void R_InitBrightmaps ()
 {
 	int lump = W_CheckNumForName("BRGHTMPS");
+
+	brightmaps = default_brightmaps;
+
 	if (lump >= 0)
 	{
 		const char *data = W_CacheLumpNum(lump, PU_CACHE);
@@ -1104,6 +1107,9 @@ void R_InitBrightmaps ()
 		R_BrightmapForSprite = R_BrightmapForSprite_None;
 		R_BrightmapForFlatNum = R_BrightmapForFlatNum_None;
 		R_BrightmapForState = R_BrightmapForState_None;
+
+		brightmaps = true;
+
 		return;
 	}

rfomin · 2022-12-17T14:06:36Z

patch

So if we turn off the brightmaps after loading the level, the textures are already cached and won't change, right? It is currently possible to disable brightmaps for sprites on the fly and for textures after a level reload, this is confusing.

Wait, no, there must be some bug in the patch, without it the switch works fine. I have a feeling we have already redesigned the DISABLE_ITEM system 😄 Can I postpone this logic a bit?

fabiangreffrath · 2022-12-17T14:13:58Z

Oh, right. We should improve on that.

rfomin · 2022-12-17T14:18:46Z

Oh, right. We should improve on that.

No, without a patch it seems to work fine. I suggest we defer the "bloodcolor-like" logic after we revisit the DISABLE_STRICT etc system.

fabiangreffrath · 2022-12-17T15:29:19Z

No, without a patch it seems to work fine. I suggest we defer the "bloodcolor-like" logic after we revisit the DISABLE_STRICT etc system.

Well, okay.

JNechaevsky · 2022-12-18T12:13:20Z

@bradharding, @rfomin yours attention is needed, It happens same way in DR too. If I have this lump structure, it works just fine:

BRIGHTMAP FULLBRIGHT 1-255

TEXTURE BRONZE2 FULLBRIGHT DOOM2
TEXTURE PIPE4 FULLBRIGHT DOOM2

Hovewer, if game version is omitted, i.e.:

BRIGHTMAP FULLBRIGHT 1-255

TEXTURE BRONZE2 FULLBRIGHT
TEXTURE PIPE4 FULLBRIGHT

Only the first one brightmap is working:

Also, current implementation supports only wall textures, not sprites/states (STbar weapons)/flats?

fabiangreffrath · 2022-12-18T14:01:35Z

Hovewer, if game version is omitted,
Only the first one brightmap is working:

I guess that's because in the second case the first line is already fully processed after the FULLBRIGHT keyword, so the parser is already on the second line after that and calling U_GetNextLineToken(s) will cause it to skip that line.

Also, current implementation supports only wall textures, not sprites/states (STbar weapons)/flats?

Yes. Textures are processed differently from sprites/states/flats. We have an array of size numtextures which contains a pointer to a brightmap for each texture available in the game. We would either have to do the same for sprites/states/flats, or we would have to populate some pendant to an unordered_list in C++. Currently, we determine the brightmaps for sprites/states/flats by means of hard-coded switch/case trees.

rfomin · 2022-12-18T16:47:09Z

I guess that's because in the second case the first line is already fully processed after the FULLBRIGHT keyword, so the parser is already on the second line after that and calling U_GetNextLineToken(s) will cause it to skip that line.

Yep, fixed in b694f5a

So should we add sprites/states/flats to BRGHTMPS? Then it won't be compatible with Doom Retro.

fabiangreffrath · 2022-12-18T17:49:20Z

So should we add sprites/states/flats to BRGHTMPS? Then it won't be compatible with Doom Retro.

Sounds more like Doom Retro's problem than ours. 😁

rfomin · 2022-12-19T04:04:55Z

Test lump:

BRIGHTMAP FULLBRIGHT 0-255
BRIGHTMAP greenonly3 112-123
SPRITE BAR1 greenonly3
FLAT FLOOR0_1 FULLBRIGHT

Not sure what to do with weapon states, how should we name them?

fabiangreffrath · 2022-12-19T05:20:25Z

Not sure what to do with weapon states, how should we name them?

Well, why not just STATE? Or PSPRSTATE?

rfomin · 2022-12-19T05:29:26Z

Not sure what to do with weapon states, how should we name them?

Well, why not just STATE? Or PSPRSTATE?

I meant how to name the states themselves. For example, we have a BFGG sprite and S_BFG1 .. S_BFG4 states, but how would the user know the names S_BFG1 .. S_BFG4 without the source code?

rfomin · 2022-12-22T11:42:43Z

Can we also have comments in BRGHTMPS lumps?

Yes, C-style comments (both /* */ and //) already work, they are built into u_scanner.c.

fabiangreffrath · 2022-12-22T11:45:39Z

Perfect!

fabiangreffrath · 2023-01-02T17:48:36Z

How do we proceed here?

rfomin · 2023-01-03T11:11:17Z

How do we proceed here?

I'll work on this tomorrow, wanted to finish the vcpkg stuff first.

For Doom Retro compatibility

autoload/hacx.wad/brghtmps.lmp

fabiangreffrath · 2023-01-09T09:42:37Z

autoload/doom.wad/brghtmps.lmp

@@ -0,0 +1 @@
+TEXTURE SW2STON2 REDONLY


I'd prefer if we could have an optional fourth parameter on each line to indicate if a brightmap applies to Doom 1 or 2 (no such parameter would imply "both"). This way we would (1) keep our format compatible to Doom Retro's (which is the purpose of this change, c.f. PR title) and (2) have less external files to rely on which in turn rely on the IWAD file names themselves (remember my concerns regarding bfgdoom.wad, udoom.wad, freedoom1.wad, d2.wad, etc.).

Alternative approach: How about we support autoload directories based on the internal gamemission name (i.e. doom, doom2, pack_tnt, pack_plut, etc.)? Then we'd end up with only one pack_rekkr directory for example and would be independent of the actual IWAD file name - because game mission is detected based on content if in doubt (c.f. CheckIWAD()).

At this point, we can make a zip/wad resource file with all the optional content and identify everything by hash. But I like the current system, I think the cases of udoom.wad d2.wad are very rare.

Is there anything that speaks against supporting the fourth field in the BRGHTMPS lump lines to differentiate between Doom 1 and 2? We could have a single brghtmps.lmp in autoload/doom-all and let the parser do the rest.

Okay, we can get rid of the brghtmps.lmp files in the doom.wad and doom2.wad directories, but we'll keep the rest of the files, right?

In general, yes. Though, we could also get rid of the file in the tnt.wad directory and merge it into the one in doom-all. The textures which get assigned a brightmap in this file do not even exist in any other IWAD.

I've added DOOM, DOOM2 and DOOM|DOOM2 optional fields. I believe this PR is complete.

fabiangreffrath · 2023-01-09T09:44:18Z

autoload/chex.wad/brghtmps.lmp

+BRIGHTMAP CHEXREDGREEN 45,112-118,174-177
+
+TEXTURE BIGDOOR1 GREENONLY3
+// {"BIGDOOR4", DOOM1AND2, greenonly3}, // C1: some stray green pixels, C2: many stray green pixels


I'd prefer if these were converted from C syntax to the actual BRGHTMPS lump format, so that in theory it would be sufficient to just uncomment these lines.

I'm not sure about Chex 2, is it Doom 2 or Doom 1? I will check later.

Chex 2 is a PWAD to Chex 1, i.e. Doom 1.

However, in the context of brightmap parsing, calling Chex 2 as Doom 2 to differentiate it from Chex 1 is unambigious.

Should we make chex-all, chex.wad and chex2.wad folders? 😄

We could. But, since chex2.wad is always loaded after chex.wad, its BRGHTMPS entries should override the ones for chex.wad anyway, right? Or, we could introduce a CHEX1|CHEX2 field in the BRGHTMPS spec if you don't like misusing the DOOM1|DOOM2 namespace. 😉

autoload/tnt.wad/brghtmps.lmp

src/r_bmaps.c

…ement

support BRGHTMPS lump from Doom Retro

ac74993

rfomin commented Dec 16, 2022

View reviewed changes

src/r_bmaps.c Outdated Show resolved Hide resolved

fix gcc build

db19de5

rfomin mentioned this pull request Dec 16, 2022

[Minor Question] The BRGHTMPS Lump bradharding/doomretro#761

Open

fix declarations

f53ea89

add const

e8a86c9

rfomin added 2 commits December 18, 2022 23:33

fix skipping optional DOOM1|DOOM2 flags

b694f5a

u_scanner: make internal functions static

95bd8e6

fix static declarations

199850d

rfomin added 2 commits December 19, 2022 10:53

add support for SPRITE and FLAT

ad7c367

remove unused function

ca4cf40

add brightmaps checks

1417bb1

rfomin added 7 commits January 7, 2023 12:57

Merge branch 'master' into brghtmps

fac8a25

scan multiple BRGHTMPS lumps, move data to autoload directory

98bd4ce

fix cppcheck

5503886

remove unnecessary BIGHTMAP declarations, formatting

e46b9e3

do not scan twice (BRIGHTMAP must be defined before use)

3aacf27

For Doom Retro compatibility

rename R_InitBrightmaps->R_ScanBrightmaps

4541e9f

implement states

91ac161

rfomin commented Jan 8, 2023

View reviewed changes

autoload/hacx.wad/brghtmps.lmp Show resolved Hide resolved

rfomin requested a review from fabiangreffrath January 8, 2023 15:31

rfomin added 2 commits January 9, 2023 14:22

skip BRGHTMPS lumps in demo footer

d761728

must get integers in ReadColormask()

7a75078

fabiangreffrath reviewed Jan 9, 2023

View reviewed changes

autoload/tnt.wad/brghtmps.lmp Show resolved Hide resolved

rfomin added 3 commits January 9, 2023 18:08

Merge branch 'master' into brghtmps

59c8124

convert comments in hacx.wad/brghtmps.lmp

b31547e

add chex2.wad directory, add NOBRIGHTMAP special case

3c5ee4a

fabiangreffrath reviewed Jan 9, 2023

View reviewed changes

src/r_bmaps.c Show resolved Hide resolved

rfomin added 5 commits January 10, 2023 16:51

implement optional DOOM|DOOM2 fields

c12278f

search from the end of the array to find the latest version of the el…

a9d8ddf

…ement

fix cppcheck

8773648

generalize arrays

7122f70

fix cppcheck

54ea0d4

fabiangreffrath approved these changes Jan 11, 2023

View reviewed changes

rfomin merged commit 88a94d2 into fabiangreffrath:master Jan 11, 2023

rfomin deleted the brghtmps branch January 11, 2023 14:31

support BRGHTMPS lump from Doom Retro #846

support BRGHTMPS lump from Doom Retro #846

Conversation

rfomin commented Dec 16, 2022

fabiangreffrath commented Dec 16, 2022

rfomin commented Dec 16, 2022

fabiangreffrath commented Dec 16, 2022 • edited Loading

rfomin commented Dec 16, 2022

fabiangreffrath commented Dec 16, 2022

fabiangreffrath commented Dec 17, 2022

fabiangreffrath commented Dec 17, 2022

rfomin commented Dec 17, 2022 • edited Loading

fabiangreffrath commented Dec 17, 2022

rfomin commented Dec 17, 2022

fabiangreffrath commented Dec 17, 2022

JNechaevsky commented Dec 18, 2022

fabiangreffrath commented Dec 18, 2022

rfomin commented Dec 18, 2022

fabiangreffrath commented Dec 18, 2022

rfomin commented Dec 19, 2022 • edited Loading

fabiangreffrath commented Dec 19, 2022

rfomin commented Dec 19, 2022

rfomin commented Dec 22, 2022

fabiangreffrath commented Dec 22, 2022

fabiangreffrath commented Jan 2, 2023

rfomin commented Jan 3, 2023

fabiangreffrath Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiangreffrath commented Dec 16, 2022 •

edited

Loading

rfomin commented Dec 17, 2022 •

edited

Loading

rfomin commented Dec 19, 2022 •

edited

Loading

fabiangreffrath Jan 9, 2023 •

edited

Loading