Crash while displaying monsters' names in RM2k battle #419

Closed
scurest opened this Issue Feb 18, 2015 · 6 comments

Projects

None yet

4 participants

@scurest
Contributor
scurest commented Feb 18, 2015

If you enter a battle in an RM2k game and press F12 to return to the title screen while the monsters' names are being displayed (eg. "Slime encountered"), and then start or load a game, the next time you enter a battle, the Player will crash when it tries to display the monsters' names.

Probably caused by the static vars in DisplayMonstersInMessageWindow not being reset, so the enemy iterator is bad. I think making them member variables would fix it.

@Ghabry
Member
Ghabry commented Feb 18, 2015

F12 resulted already in many crashes... We can't really get this right :/

I don't really see why static variables are a problem here because Scene_Battle is an instance so the statics are destroyed when a new battle starts?

@fdelapena
Member

Thanks @scurest for the find! I'm not sure if related, but there are some warnings when building with gcc with -Wunused-variable due to static variables, here is the list:

game_battle.h:70:13: warning: 'Game_Battle::turn' defined but not used [-Wunused-variable]
  static int turn;
             ^
game_battle.h:71:14: warning: 'Game_Battle::message_is_fixed' defined but not used [-Wunused-variable]
  static bool message_is_fixed;
              ^
game_battle.h:72:13: warning: 'Game_Battle::message_position' defined but not used [-Wunused-variable]
  static int message_position;
             ^
@scurest
Contributor
scurest commented Feb 20, 2015

@Ghabry statics shouldn't ever be destroyed, their lifetime is forever. I mean, something like

struct A { int get() { static int g = 0; return g++; } };
int main() {
    { A a1; std::cout << a1.get(); }
    { A a2; std::cout << a2.get(); }
}

will print 01, if that's what you meant.

There's also a similar, but less severe, bug if you press F12 during the battle action processing where a skill animation won't play the next time you get into a battle, caused by the static in ProcessBattleAction.

@glynnc
Contributor
glynnc commented Feb 20, 2015

@fdelapena that's a different use of "static".

C uses "static" for 2 different purposes:

  1. If a local variable has the "static" qualifier, the variable has static storage (a single instance which is stored in the data segment and persists for the lifetime of the process) rather than automatic storage (the variable is allocated on the stack when the function is entered and destroyed when it returns; recursive calls will result in each call having its own instance).
  2. If a global ("file scope") variable has the "static" qualifier, the symbol isn't exported, so it can only be referenced from within the compilation unit (a source file and all included files) in which it is declared. Apart from anything else, this allows the compiler to warn if it's unused.

C++ also allows class members to be "static", which is similar to case 1 above, i.e. a static member is essentially a global variable but inside the class' namespace, rather than being a field within each instance.

This issue relates to case 1; the gcc warnings relate to case 2.

As @scurest points out, the problem is the use of the static variable "first" in Scene_Battle_Rpg2k::DisplayMonstersInMessageWindow(). This is initialised the first time the function is called (in practice, it will be initialised during compilation, and already have the value "true" by the time that main() is entered), and its value persists until explicitly changed. "Resetting" the program with F12 won't reset this variable (or any other static variable), nor will starting another battle.

I suggest initialising enemy_iterator in the constructor. Or in the scene's Start() method (this would need to be overridden rather than inheriting it from Scene_Battle).

@Ghabry
Member
Ghabry commented Feb 20, 2015

So I learned something new after nearly 10 years :D. That's exactly what I assumed @scurest....

Static initializations are only done once even in member functions. Crap, I didn't knew this, I thought they are initialized once per object instance because the static is in a member function :(

Okay I won't use static variables anymore -.-

@fdelapena
Member

Thank you @glynnc and @scurest for the comprehensive explanation, I didn't know about these behaviour details too.

@fdelapena fdelapena added this to the 0.3 milestone Feb 23, 2015
@fdelapena fdelapena modified the milestone: 0.3.1, 0.3 Apr 29, 2015
@Ghabry Ghabry modified the milestone: 0.3.1, 0.4 Sep 6, 2015
@Ghabry Ghabry added a commit to Ghabry/easyrpg-player that referenced this issue Dec 15, 2015
@Ghabry Ghabry Get rid of static vars in battle. Fixes #419 b6144ca
@Ghabry Ghabry added a commit to Ghabry/easyrpg-player that referenced this issue Dec 16, 2015
@Ghabry Ghabry Get rid of static vars in battle. Fixes #419 4cc7fec
@Ghabry Ghabry closed this in 7eeb891 Dec 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment