I_Error messages for errors can be more informative

[JNechaevsky]

The idea is to review and make I_Error messages for errors more informative. For example, if we have a Bad V_DrawPatch, we can indicate which exactly patch is making a problem. In R_MapPlane we can "slightly deobfuscate" message: R_MapPlane: 1, 2 at 3, to R_MapPlane: x1: 1, x2: 2 at y: 3, an so on.

Quote from Fabian:

On the one hand, Choco has an interest in keping the error messages identical to the original, on the other hand error messages are some kind of user interface - and this should be as easy to understand as possible

So, can these I_Error changes be done in Chocolate Doom?

By the request of @fabiangreffrath, please see: https://github.com/fabiangreffrath/crispy-doom/issues/349.

[fragglet]

I don't think choco needs to strictly adhere to the original error messages - in fact there are already several cases where it shows more detail than the original vanilla error messages. So I say go for it and add more detail if you think it's helpful.

One suggestion I would make is that vanilla error messages should link to the Doom wiki. For example, if you get a visplane overflow message, probably we should provide a link to the wiki article on visplane overflows to give some context.

[JNechaevsky]

Okay, here's the list. I'm suggesting to start from Doom only, and if thing goes well, we can continue with other three games. Any suggestion is highly appreciated!

v_video.c

V_DrawPatch (and others patch drawing functions, like V_DrawPatchFlipped)

Original: Bad V_DrawPatch Suggested: Bad V_DrawPatch "M_DOOM"

This is mostly common error I believe, and indication of which exactlly patch is causing an error will be extremely helpful. An obvious approach is to extend error code to this:

I_Error("Bad V_DrawPatch %p", patch);

But no, it's not working this way, because instead of patch name, a numerical number will be shown. Perhaps, some additional work must be done in W_CacheLumpName, but sorry, I have no idea what to do.

Testing WAD: badpatch.zip (contains null-lenght M_DOOM lump)

g_game.c

G_DoSaveGame

Original: Savegame buffer overrun Suggested: Savegame buffer overrun. Save file limit can be disabled in the Compatibility menu of Setup executable (something like that).

G_CheckDemoStatus

Original: timed 666 gametics in 777 realtics (888 fps) Suggested: timed 666 gametics in 777 realtics (888 average fps)

Timedemo is an awesome tools for benchmarking, but someone may not know/understand what actually "fps" meaning here.

p_doors.c

EV_VerticalDoor

Original: EV_VerticalDoor: DR special type on 1-sided linedef Suggested: EV_VerticalDoor: DR special type on 1-sided linedef number 666

I.e., indication which linedef is causing an error.

p_enemy.c

P_Move

Original: Weird actor->movedir! Suggested: P_Move: Weird actor->movedir of actor 666 (thing number)!

Not sure is it reproducible at all without modifying a source code, but just as idea.

P_NewChaseDir

Original: P_NewChaseDir: called with no target Suggested: P_NewChaseDir: called by thing number 666 with no target

Who exactly is calling?

p_inter.c

P_TouchSpecialThing

Original: P_SpecialThing: Unknown gettable thing Suggested: P_SpecialThing: Unknown gettable thing №666

Which thing is unknown? Indicate it's number.

p_map.c

PTR_SlideTraverse

Original: PTR_SlideTraverse: not a line? Suggested: PTR_SlideTraverse: linedef 666 not a line?

Indicate which linedef is not a line.

p_mobj.c

P_SetMobjState

Original: P_SetMobjState: Infinite state cycle detected! Suggested: P_SetMobjState: Infinite state cycle detected in mobj MT_ZOMBIE

Who is making this mess?

P_SpawnMapThing

Original: P_SpawnMapThing: Unknown type 666 at (128, 256) Suggested: P_SpawnMapThing: Unknown type 666 at (x: 128, y: 256)

Indicate that numbers in parentheses are actually map coordinates.

p_plats.c

P_AddActivePlat

Original: P_AddActivePlat: no more plats! Suggested: P_AddActivePlat: MAXPLATS limit (30) reached. No more plats!

P_RemoveActivePlat

Original: P_RemoveActivePlat: can't find plat! Suggested: P_RemoveActivePlat: can't find plat in sector 666!

Not sure about it. It's looking for thinker, not a sector...

p_sight.c

P_CrossSubsector

Original: P_CrossSubsector: ss %i with numss = %i Suggested: P_CrossSubsector: subsector number: 1 with numsubsectors = 2

Erm.

p_switch.c

P_StartButton

Original: P_StartButton: no button slots left! Suggested: P_StartButton: MAXBUTTONS limit (30) reached. No button slots left!

r_bsp.c

R_Subsector

Original: R_Subsector: ss %i with numss = %i Suggested: R_Subsector: subsector number: 1 with numsubsectors = 2

Erm-erm.

r_draw.c

R_DrawColumn, R_DrawColumnLow, R_DrawFuzzColumn, R_DrawFuzzColumnLow, R_DrawTranslatedColumn, R_DrawTranslatedColumnLow

Original: R_DrawColumn: 11 to 22 at 33 Suggested: R_DrawColumn: dc_yl: 11 to dc_yh: 22 at dc_x: 33

R_DrawSpan, R_DrawSpanLow

Original: R_DrawSpan: 11 to 22 at 33 Suggested: R_DrawSpan: ds_x1: 11 to ds_x2: 22 at ds_y: 33

r_plane.c

R_MapPlane

Original: R_MapPlane: 11, 22 at 33 Suggested: R_MapPlane: x1: 11, x2: 22 at y: 33

R_FindPlane

Original R_FindPlane: no more visplanes Suggested: R_FindPlane: MAXVISPLANES limit (128) reached. No more visplanes.

But not sure about "R_DrawPlanes: drawsegs overflow (%" PRIiPTR ")" in R_DrawPlanes then. So what is a visplane, anyway?

r_segs.c

R_StoreWallRange

Original: Bad R_RenderWallRange: 11 to 22 Suggested: Bad R_StoreWallRange: start: 11 to stop: 22

[fabiangreffrath]

Thank you for your wording sugestions, @JNechaevsky !

However, I have two objections:

(1) It is much easier to review and discuss code changes if they are submitted as a pull request, because then we could refer to the changes line-by-line instead of refering to the same single large comment again and again.

(2) I actually prefer @fragglet 's proposal of adding links to the relevant Doomwiki pages to the error messages. So, instead of changing Doom's pristine error messages by subjective discretion, we merely extend them. And since we all have access to Doomwiki, it would be in our own responsibility to make these pages as clear and informative as possible and help them "translate" from the original error message to something more easy to understand.