Jump to content
Search In
  • More options...
Find results that contain...
Find results in...
fabian

Is vissprite_t savegame/demo/netgame-critical?

Recommended Posts

Hi again,

I have a question for the more experienced Doom source coders out there:

I'd like to implement a feature in Crispy Doom that requires an additional element in player_t (struct player_s). Is it safe to add it there or is this struct used in savegames, demos or netgames. Will this break compatibility?

- Fabian

Share this post


Link to post

At the very least it will break savegame compatibility. Adding an extra struct to a field isn't likely* to cause things like netgame or demo desyncs until you're using it to change the game behavior in some way

* - There are various Vanilla demo compatibility corner cases where a buffer is overflowed and you start reading or writing into another unrelated buffer. I don't think player_t is one of the ones affected as far as I know.

Share this post


Link to post
fraggle said:

At the very least it will break savegame compatibility. Adding an extra struct to a field isn't likely* to cause things like netgame or demo desyncs until you're using it to change the game behavior in some way

* - There are various Vanilla demo compatibility corner cases where a buffer is overflowed and you start reading or writing into another unrelated buffer. I don't think player_t is one of the ones affected as far as I know.

There's at least one in Doom. entryway knows about it, but, I couldn't describe it to you.

Strife also has a potential one, although you have to modify the EXE with SeHackEd to actually trigger it :P That's neither here nor there, of course. I just had to mention it.

Share this post


Link to post
fraggle said:

At the very least it will break savegame compatibility. Adding an extra struct to a field isn't likely* to cause things like netgame or demo desyncs until you're using it to change the game behavior in some way


Thank you for your replies, I have managed to implement the feature without introducing the new field in the struct.

FYI, I wanted to implement a centered "A Secret Is Revealed!" message as an optional feature in Crispy Doom and added another "char *secretmsg" element for it to the struct. However, now I changed the code to write the string into the general "char *message" element and added some special casing if !strcmp(message, STSTR_SECRETFOUND).

Quasar said:

There's at least one in Doom. entryway knows about it, but, I couldn't describe it to you.


Does this have to do with REJECT lumps, maybe?

Edit: No, it doesn't, I have mixed this up. But player_t has the following comment in PrBoom+:

  // e6y
  // All non original (new) fields of player_t struct are moved to bottom
  // for compatibility with overflow (from a deh) of player_t::ammo[NUMAMMO]

Share this post


Link to post

YMMV. In the general case no, because vissprite_t is not a structure that's used in a two-way interaction with the game's gameplay code, only the renderer handles it. It's not read/written to disk, nor copied directly over other structs in a way that would be affected by the size() macro. In addition, even in vanilla Doom, it's statically allocated and the vissprite_t list is never overflowed, nor can it cause engine crashes.

Then again, it depends on what this "extra field" is. Just an integer? A pointer to a struct or object? Can this object be it modified by the renderer itself? Is this object visible/interacting with other parts of the code?

Share this post


Link to post
fabian said:

Edit: No, it doesn't, I have mixed this up. But player_t has the following comment in PrBoom+:

  // e6y
  // All non original (new) fields of player_t struct are moved to bottom
  // for compatibility with overflow (from a deh) of player_t::ammo[NUMAMMO]

Cute/funny thing is, that's NOT the appropriate way to fix such an overflow because you're allowing it to rely on undefined behavior. Compiler may pad the structure differently than Watcom did.

To emulate an in-structure overflow properly, you need to write a routine that puts the overflowed values into the fields that you know they overflowed into *as the structure was laid out in the vanilla EXE as compiled by Watcom*.

I told entryway about this before; I am sad to see that he continues to ignore my advice.

Share this post


Link to post
Maes said:

YMMV. In the general case no, because vissprite_t is not a structure that's used in a two-way interaction with the game's gameplay code, only the renderer handles it. It's not read/written to disk, nor copied directly over other structs in a way that would be affected by the size() macro. In addition, even in vanilla Doom, it's statically allocated and the vissprite_t list is never overflowed, nor can it cause engine crashes.


Thank you very much, Maes, for this information. That is exactly the technical background that I was looking for.

Then again, it depends on what this "extra field" is. Just an integer? A pointer to a struct or object? Can this object be it modified by the renderer itself? Is this object visible/interacting with other parts of the code?


The extra field is a byte* pointing to a color translation lookup table, if the object that the sprite belongs to is going to have its color changed (i.e. blood sprites for different kinds of monsters), it points to NULL else. The code is all in r_things.c and the field is not accessed from outside. The field is written in R_ProjectSprite() and read in R_DrawVisSprite().

Share this post


Link to post
Quasar said:

Cute/funny thing is, that's NOT the appropriate way to fix such an overflow because you're allowing it to rely on undefined behavior. Compiler may pad the structure differently than Watcom did.


You are right. The following comment can be found in Chocolate Doom's g_game.c:

    // Set par time. Doom episode 4 doesn't have a par time, so this
    // overflows into the cpars array. It's necessary to emulate this
    // for statcheck regression testing.
However, I just checked with gcc [(Debian 4.8.2-16) 4.8.2] and actually it was the other way round: The par time for map33 (the extra map found in the BFG Edition doom2.wad) was taken from pars[0][0], i.e. from the array for Doom 1, which is defined *before* cpars[] in that file.

Share this post


Link to post
Quasar said:

Compiler may pad the structure differently than Watcom did.


By the way, you can prevent GCC from reordering most structures by the following variable:

-fno-toplevel-reorder
    Do not reorder top-level functions, variables, and asm statements. Output them in the same order that they appear in the input file. [...]
http://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html

Share this post


Link to post

I was going to reply to this but Maes already summarized most of what I was going to say. In general, just adding a field to a structure won't hurt compatibility (not taking into account things like buffer overruns). It's how the field is used and whether it affects the gameplay that matters the most.

If you haven't seen statcheck before then I recommend setting that up so you can do regression testing.

Share this post


Link to post
fabian said:

By the way, you can prevent GCC from reordering most structures by the following variable:

There are almost always compiler switches to force assumptions that you're not supposed to make according to the language spec to be true, but relying on them makes your code less portable, less secure, and possibly slower as well in many instances (you are preventing an entire class of optimizations by using that flag, for example). I'd never personally advise application of compiler switches in place of code that is strictly well-defined under the language standard.

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
×