Skip to content

Commit

Permalink
Restore part of "Trim edict list on load" in order to fix 'autofastload'
Browse files Browse the repository at this point in the history 
Otherwise autofastload leaks edicts, eventually aborting game when max_edicts is reached
BEWARE: this _may_ break old savegames !
  • Loading branch information
@temx @vsonnier
temx authored and vsonnier committed Jul 20, 2024
1 parent 0314204 commit 9e3cfe2
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 45 deletions.
38 changes: 14 additions & 24 deletions Quake/host_cmd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1306,7 +1306,7 @@ static void Host_Loadgame_f (void)
const char *data;
int i;
edict_t *ent;
int entnum;
int entnum, lastusedent;
int version;
float spawn_parms[NUM_TOTAL_SPAWN_PARMS];
qboolean was_recording = cls.demorecording;
Expand Down Expand Up @@ -1446,7 +1446,8 @@ static void Host_Loadgame_f (void)
PR_ClearEdictStrings ();

// load the edicts out of the savegame file
entnum = -1; // -1 is the globals
qcvm->time = 0; // mark freed edicts for immediate reuse
entnum = lastusedent = -1; // -1 is the globals
while (*data)
{
while (*data == ' ' || *data == '\r' || *data == '\n')
Expand Down Expand Up @@ -1571,22 +1572,7 @@ static void Host_Loadgame_f (void)
if (entnum < qcvm->num_edicts)
{
if (ent->free)
{
if (qcvm->free_edicts_head == ent)
{
assert (!ent->prev_free);
qcvm->free_edicts_head = ent->next_free;
}
if (qcvm->free_edicts_tail == ent)
{
assert (!ent->next_free);
qcvm->free_edicts_tail = ent->prev_free;
}
if (ent->prev_free)
ent->prev_free->next_free = ent->next_free;
if (ent->next_free)
ent->next_free->prev_free = ent->prev_free;
}
ED_RemoveFromFreeList (ent);
ent->free = false;
ent->next_free = NULL;
ent->prev_free = NULL;
Expand All @@ -1601,15 +1587,23 @@ static void Host_Loadgame_f (void)

// link it into the bsp tree
if (!ent->free)
{
SV_LinkEdict (ent, false);
lastusedent = entnum;
}
}

entnum++;
}

qcvm->time = time;
for (i = entnum; i < qcvm->num_edicts; i++)
for (i = lastusedent + 1; i < q_max (qcvm->num_edicts, entnum); i++)
{
ED_Free (EDICT_NUM (i));
ED_RemoveFromFreeList (EDICT_NUM (i));
memset (EDICT_NUM (i), 0, qcvm->edict_size);
}
qcvm->time = time;
qcvm->num_edicts = lastusedent + 1;

if (fastload)
{
Expand All @@ -1627,10 +1621,6 @@ static void Host_Loadgame_f (void)

Send_Spawn_Info (svs.clients, true);
}
else if (entnum < qcvm->num_edicts)
Con_Warning ("Save game had less entities than map (%d < %d)\n", entnum, qcvm->num_edicts); // should be Host_Error, but try to recover

qcvm->num_edicts = q_max (qcvm->num_edicts, entnum);

Mem_Free (start);
start = NULL;
Expand Down
64 changes: 43 additions & 21 deletions Quake/pr_edict.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,33 @@ void ED_Free (edict_t *ed)
}
}

/*
=================
ED_RemoveFromFreeList
Used at load time to place edicts at a specifit spot, and to trim qcvm->num_edicts
=================
*/
void ED_RemoveFromFreeList (edict_t *ed)
{
assert (ed->free);

if (qcvm->free_edicts_head == ed)
{
assert (!ed->prev_free);
qcvm->free_edicts_head = ed->next_free;
}
if (qcvm->free_edicts_tail == ed)
{
assert (!ed->next_free);
qcvm->free_edicts_tail = ed->prev_free;
}
if (ed->prev_free)
ed->prev_free->next_free = ed->next_free;
if (ed->next_free)
ed->next_free->prev_free = ed->prev_free;
}

//===========================================================================

/*
Expand Down Expand Up @@ -738,42 +765,37 @@ For savegames
*/
void ED_Write (FILE *f, edict_t *ed)
{
ddef_t *d;
int *v;
int i, j;
const char *name;
int type;

fprintf (f, "{\n");
ddef_t *d;
int *v;
int i;
int type;

if (ed->free)
{
fprintf (f, "}\n");
fprintf (f, "{\n}\n");
return;
}

fprintf (f, "{\n");

for (i = 1; i < qcvm->progs->numfielddefs; i++)
{
d = &qcvm->fielddefs[i];
name = PR_GetString (d->s_name);
j = strlen (name);
if (j > 1 && name[j - 2] == '_')
continue; // skip _x, _y, _z vars
type = d->type;
assert (!!(type & DEF_SAVEGLOBAL) == (strlen (PR_GetString (d->s_name)) > 1 && PR_GetString (d->s_name)[strlen (PR_GetString (d->s_name)) - 2] == '_'));
if (type & DEF_SAVEGLOBAL)
continue;

v = (int *)((char *)&ed->v + d->ofs * 4);

// if the value is still all 0, skip the field
type = d->type & ~DEF_SAVEGLOBAL;
for (j = 0; j < type_size[type]; j++)
{
if (v[j])
break;
}
if (j == type_size[type])
assert (type < 8 && ((type == ev_vector && type_size[type] == 3) || (type != ev_vector && type_size[type] == 1)));
if (type != ev_vector && !v[0])
continue;
if (type == ev_vector && !v[0] && !v[1] && !v[2])
continue;

fprintf (f, "\"%s\" ", name);
fprintf (f, "\"%s\"\n", PR_UglyValueString (d->type, (eval_t *)v));
fprintf (f, "\"%s\" \"%s\"\n", PR_GetString (d->s_name), PR_UglyValueString (d->type, (eval_t *)v));
}

// johnfitz -- save entity alpha manually when progs.dat doesn't know about alpha
Expand Down
1 change: 1 addition & 0 deletions Quake/progs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ void PR_Profile_f (void);

edict_t *ED_Alloc (void);
void ED_Free (edict_t *ed);
void ED_RemoveFromFreeList (edict_t *ed);

void ED_Print (edict_t *ed);
void ED_Write (FILE *f, edict_t *ed);
Expand Down

15 comments on commit 9e3cfe2

@j4reporting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does it show if the save games do not work? Only when saving?

"autoload" is "1"
]save immortal_autoload_1
Saving game to C:\Games\vkquake/immortal/immortal_autoload_1.sav...
Host_Error: NUM_FOR_EDICT: bad pointer

are new savegames still be compatible with ironwail /qs /qss ?

@j4reporting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'old' savegame stats 133 kills 16 secrets (skill 1)

@vsonnier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Host_Error: NUM_FOR_EDICT: bad pointer

Exactly.

are new savegames still be compatible with ironwail /qs /qss ?

I have no idea. This is the same format as before, except the amount of saved entities is "optimized", so to speak so I would say the problem is only between the old and new vkQuake saves.

In any case, the compatibility between QSes is already shaky because there are a shitton of optional fields loaded under the section // QuakeSpasm extended savegame

@vsonnier
Copy link
Collaborator

@vsonnier vsonnier commented on 9e3cfe2 Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'old' savegame stats 133 kills 16 secrets (skill 1)

Do not feel compelled to change now, there is no performance gain with that latest version as you know.

On the other hand, there is a piece of code by @temx in the Trim edict on load commit that was supposed to assure 'compatibility with old saves' (whatever that means) that I purposfully do not add, because I thought it was a super-niche problem. Do you think it is worth adding it back ?

@vsonnier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've added back the old savegame compatibility trick, If you want to test it.

@j4reporting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is a good change for a minor release update.

could you add this extra code in an extra branch pls?
it's possible that the savegame will break later anyway if I continue playing normally.

save game compatibility:
most of the time I was able to load a savegame in other engines as long as I made sure that all extentions were available
i.e r_fteparticles 1 vkquake > qss (AD maps)
r_fteparticles 0 vkquake > ironwail (AD maps )

@j4reporting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately still get the same errors when saving.

I have two more savegames from this play test (immortal v1.5) and the one with 102 kills fails as well and the one quite early ( 57 kill) seem to work. At least no error when saving.

I tried the same with two AD savegames and they seem to work (load / save test )
save_immortal_1.5.zip

I guess immortal is special in every possible way...

@vsonnier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

save_immortal_1.5.zip

Well here with actually v1.7 installed, all 3 saves load !

@j4reporting
Copy link
Contributor

@j4reporting j4reporting commented on 9e3cfe2 Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loading is not the problem. It fails when saving!

load  s3.sav

save immortal_autoload_1
Saving game to C:\Games\vkquake/immortal/immortal_autoload_1.sav...
Host_Error: NUM_FOR_EDICT: bad pointer

@j4reporting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's pure luck that these savegames load with immortal 1.7. And there is a high risk they will have game breaking bugs. Previous updates didn't load old savegames successfully at all.

@vsonnier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see the same thing as you. Now I'm bound to play The Immortal Lock until it crashes, it seems.
If I reproduce the bug then I'll revert the Trim Edict on load things. This is a pity because without it 'autofasload' is broken (leaks edicts like crazy, which in turns inflates the ones saved, until you are stuck and cannot continue) and it would be really usefull for this kind of monster precisely.

@j4reporting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's more worring, could this also happen if you start fresh without loading an old savegame?

@vsonnier
Copy link
Collaborator

@vsonnier vsonnier commented on 9e3cfe2 Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK with 0416d90 I can now save in Immortal Lock.

The 'Ultra-fast' loading mode may starts leaking edicts again but in fact it entirely depends on the level : Immortal Lock for instance looks unnaffected. (looking at edictcount before and after loading a level)

Now that I have tested the few mods I have installed, the only pathological case I still have is Replicon : leaking like crazy.

@j4reporting
Copy link
Contributor

@j4reporting j4reporting commented on 9e3cfe2 Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'Ultra-fast' loading mode starts leaking edicts again, but in fact it entirely depends on the level

I might have missed here something. What's the diference between autoload 2 and autoload 3?
they look the same to me. Both restart immediately.

ok got it. fastload instead of load

An opposite example is Replicon, leaking like crazy : for such level you have to disable the Ultra-fast (a.k.a autofastload) entirely.

ok I'll try this later. but immortal is the 1st map I would consider using the fastload feature

@vsonnier
Copy link
Collaborator

@vsonnier vsonnier commented on 9e3cfe2 Jul 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autoload 3 = autoload 2 (a.k.a Fast) + autofastload 1 (hidden from user, default = 0) which is the equivalent of making the command load SAVE (including through menu) behave like fastload SAVE all the time.
The loading speed difference is huge, you litterally teleports to the previous place.

You can always bind a shortcut like:

bind F6 "echo Quickloading...; wait; fastload quick"

but this is not public knowledge.

Please sign in to comment.