[r2867+] Player class set to random

[DBThanatos]

Tested also with several svn versions up to r2940, with same results.

If you go to player setup, and set the player to an specific type, press esc, and return to player setup, the player will be set to random.

You can test this behavior with hexen. Set player class on the "Player setup", then enter any level through the console, and you'll see the player class will be actually random.

This bug appeared somewhere between r2837 (no issues) and r2867(2869). Sorry I cant narrow it down further, but those are the only ones I have from that period.

Apparently, this was fixed before, but it came back.

[Player701]

OMG. Confirmed.

Setting the class through the console variable "playerclass" works fine, though.

[Graf Zahl]

That's because the new menu code doesn't make any permanent settings until you actually start the level.

This was one serious issue with the old code. It set global variables at a time when it wasn't even clear that the selection being made will actually get used. Now the selections of the 'start game' menu chain are stored locally and only applied when the final menu in this chain is terminated and the level being started.

In fact, selecting something in this menu should not have an effect on the player setup screen until a game with the selected player class is started and that's precisely what happens now.


BTW, the link to another bug report is something different.

[DBThanatos]

So, that means that now we cant rely on the player setup menu to set the class, but actually use the regular player selection screen (like in hexen)? If so, that's fine, but I wonder, in which case the player setup screen (the "player class" part) would come useful if at all?

[Enjay]

Unless I'm missing something, this change does seem to make the ability to change player class on the player setup screen irrelevant. I don't usually change player class mid game so I'm not too familiar with how useful that menu option is and the only time that I have needed to use it is, of course, with AEoD.

If the player setup menu is not longer a suitable place to change the player class, then, IMO, that ability should be removed from that menu. It could, perhaps, still report which player class is active but not allow the class to be changed (seeing as how the change is ignored anyway and set to random instead - that is if my interpretation of what appears to be the current behaviour is correct).

[Graf Zahl]

For single player games in a mod that has a player selection menu, yes. But there's still situations where using that menu makes sense.

Honestly, I think what Grubber made back then was very poorly thought out. The player setup menu should never have been influenced by anything that's done in the single player startup menus. The setting in there should have been for multiplayer and direct console starts only. But it was all mashed together in the same global variables and now, years later, it's no longer possible to clean this up and make it robust.

[Grubber]

Well, what I did then was simply extending what was already there without making too invasive changes. You (or randy, or anyone else) had plenty of time (cca 3 months between the release of 2.0.98x and inclusion of the player class code in r250) to review the code and/or make any necessary changes. You even could throw the code away and don't use it at all. It was your decision, and I never was an official ZDoom developer, so don't blame me.

BTW:

However, and here lies the problem, Grubber used the 'display'names for purposes other than just displaying them so I have 2 choices now: Either risk breaking a handful of mods or leave it as it is which means that player class displaynames cannot be stringtable references.

You got it wrong. I didn't use displayname, it didn't exist before I added it. The code it evolved from already used it for purposes other than displaying names, before I touched it. What I did wrong was naming the property displayname, it should have been playerclassname or something like that.

I've attached the original patch so you can see for yourself that I didn't change any of the logic (at least not on purpose), just generalized it. Most of the code is actually definition of the new actor properties, addition of the properties to the decorate parser and changes to the menu (and even there I followed the original code as closely as I could).

[Graf Zahl]

Well, I don't really know, how much of that stuff was accessible to modders before the addition - it was too long ago - and I have to admit that back then I did not spot the issue. I only realized its full impact when I did the menu rewrite and ran into problems with the player class selection menu for Hexen. Of course hindsight is always clearer but from today's perspective the system as it is sure shows its problems and if I had to add it today I would make some changes.

Anyway, even if the property was called 'Playername' it would still be the same: The same name is used for internal purposes and for exposition to the player who should not care at all about engine internals. So even if it hadn't been possible to match it to the player's actor class name this should have been 2 separate properties.

I guess there's one lesson to be learned here: Making a feature with the 'least invasive' implementation is not always the best idea (again, isn't hindsight a bitch? )

[Grubber]

It sure is