Cacodemon
Register | User Profile | Member List | F.A.Q | Privacy Policy | New Blog | Search Forums | Forums Home
Doomworld Forums : Powered by vBulletin version 2.2.5 Doomworld Forums > Classic Doom > Source Ports > Is vissprite_t savegame/demo/netgame-critical?
 
Author
All times are GMT. The time now is 10:11. Post New Thread    Post A Reply
fabian
Member


Posts: 386
Registered: 12-12


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

Last edited by fabian on 04-01-14 at 07:22

Old Post 02-27-14 10:52 #
fabian is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
fraggle
Filled with the code of Doom


Posts: 7771
Registered: 07-00


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.

Old Post 02-27-14 15:28 #
fraggle is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
kb1
Member


Posts: 414
Registered: 11-06


Totally agree with all aspects of fraggle's answer.

Old Post 02-27-14 20:03 #
kb1 is offline Profile || Blog || PM || Search || Add Buddy IP || Edit || Quote
Quasar
Moderator


Posts: 6123
Registered: 08-00



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.

Old Post 03-01-14 18:35 #
Quasar is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
fabian
Member


Posts: 386
Registered: 12-12



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+:

code:
// 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]

Last edited by fabian on 04-01-14 at 07:20

Old Post 03-04-14 06:41 #
fabian is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
fabian
Member


Posts: 386
Registered: 12-12


How about vissprite_t? Does anybody know if it is safe to add a field here?

Old Post 04-01-14 07:22 #
fabian is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
Maes
I like big butts!


Posts: 12750
Registered: 07-06


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?

Old Post 04-01-14 19:43 #
Maes is offline Profile || Blog || PM || Homepage || Search || Add Buddy IP || Edit || Quote
Quasar
Moderator


Posts: 6123
Registered: 08-00



fabian said:

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

code:
// 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.

Old Post 04-01-14 20:52 #
Quasar is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
fabian
Member


Posts: 386
Registered: 12-12



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().

Old Post 04-01-14 22:32 #
fabian is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
fabian
Member


Posts: 386
Registered: 12-12



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:
code:
// 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.

Old Post 04-01-14 22:37 #
fabian is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
fabian
Member


Posts: 386
Registered: 12-12



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:
code:
-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

Old Post 04-02-14 20:07 #
fabian is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
fraggle
Filled with the code of Doom


Posts: 7771
Registered: 07-00


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.

Old Post 04-02-14 20:41 #
fraggle is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
Quasar
Moderator


Posts: 6123
Registered: 08-00



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.

Old Post 04-03-14 00:34 #
Quasar is offline Profile || Blog || PM || Email || Homepage || Search || Add Buddy IP || Edit || Quote
All times are GMT. The time now is 10:11. Post New Thread    Post A Reply
 
Doomworld Forums : Powered by vBulletin version 2.2.5 Doomworld Forums > Classic Doom > Source Ports > Is vissprite_t savegame/demo/netgame-critical?

Show Printable Version | Email this Page | Subscribe to this Thread

 

Forum Rules:
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts
HTML code is OFF
vB code is ON
Smilies are OFF
[IMG] code is ON
 

< Contact Us - Doomworld >

Powered by: vBulletin Version 2.2.5
Copyright ©2000, 2001, Jelsoft Enterprises Limited.