Rename Monster's goals

[mikolaj-pirog]

Rename goals in Monster struct to more descriptive names. This is done to improve readability and make code less cryptic.

This PR is a result of discussion in #4861

The names suggested are not the best ones: they do not cover every use case, namely goalTurning and goalGeneral is used in a custom way by some Ais, which is not reflected by their names. Because the goals are used in a various ways across monster code it is hard to come up with a great name. I think it would be best if we change the current names to something, since I believe that having descriptive name that covers most of the usage is better than having a general, but very cryptic name.

The suggestions of other names and/or ideas are more than welcome.

[AJenbo]

I don't think we can give better names to 1 and 2, but maybe 3

[FitzRoyX]

goalVar3, at least for scavengers, appears to be "remainingTicksToEat" or something, right?

[canassa]

I was thinking about how to improve that as well. In a green field project, we would probably design custom monsters as subclasses of Monster and add any extra field to the subclasses.

Perhaps we could achieve something similar by creating a set of getter and setters around those 3 variables with the goal of splitting the game logic from the storage. e.g.:

void Monster::setLeoricMoveCounter(int counter) {
	goalVar1 = direction;
}

int Monster::getLeoricMoveCounter() {
	return goalVar1;
}

void Monster::setLeoricMoveDirection(int direction) {
	goalVar2 = direction;
}

int Monster::getLeoricMoveDirection() {
	return goalVar2;
}

Then this part of LeoricAi() could be rewritten from:

if (monster.goal != MGOAL_MOVE) {
	monster.goalVar1 = 0;
	monster.goalVar2 = GenerateRnd(2);
}

into

if (monster.goal != MGOAL_MOVE) {
	monster.setLeoricMoveCounter(0);
	monster.setLeoricMoveDirection(GenerateRnd(2));
}

[AJenbo]

Maybe unions would be an option?

[DakkJaniels]

I was thinking the same thing - but I wasn't sure if there would be any time when it would be set as one variable in the union and accessed as another... I was reading that some implementations of C++ may not like that? Although if they are all the same type size, maybe it doesn't matter?

[mikolaj-pirog]

What @canassa suggested would be problematic, since we would have to implement many many member functions in monster that would do similar things.

The best solution would to be rewrite the whole monster to a nice OO, hierarchy, with abstract Monster class and particular monsters inheriting it - but that is not feasible.

I think it would best to leave the logic of AIs to specific functions, since creating setters & getters for specific actions would result in a very big monster class. The logic itself is just too convoluted.

I don't really see what others mean by using unions: split the goalSpecialAction to union of two vars? If that's it, it would introduce some ifs - but would make obvious what is being done in Ais.

[StephenCWills]

I don't really see what others mean by using unions: split the goalSpecialAction to union of two vars? If that's it, it would introduce some ifs - but would make obvious what is being done in Ais.

Unions can be used to implement something like a brute-force form of polymorphism. You can refer to the pattern used by SDL_Event to see what I mean.

https://github.com/libsdl-org/SDL/blob/c0eada20198430109f5072d13b51cf4554a1dec3/include/SDL_events.h#L621-L669

So in the LeoricAi() example, it could end up looking like...

if (monster.goal != MGOAL_MOVE) {
	monster.goalVars.leoric.moveCounter = 0;
	monster.goalVars.leoric.moveDirection = GenerateRnd(2);
}

And then loadsave.cpp could still do something like monster.goalVars.common.goalVar1.

[mikolaj-pirog]

I think I see what you mean. But wouldn't it mean we would have to implement a union of structs for every monster ai? And then every monster ai would just access its union, whenever it needs goal...? Or just a common denominator general struct and special structs within the union for monsters that do custom things? Introducing unions would also introduce manual management of them, and more complicated code. I think it's not worth it

[StephenCWills]

But wouldn't it mean we would have to implement a union of structs for every monster ai? And then every monster ai would just access its union, whenever it needs goal...?

Yes, it could very well mean that, assuming every AI uses these variables in a specific way that is distinct from how every other AI uses them.

EDIT: To be clear, you would need a struct for each AI and a single union in the Monster struct to group them all together in the same memory space.

Or just a common denominator general struct and special structs within the union for monsters that do custom things?

Ideally, common usages could be grouped by a single struct, if such common usage can be clearly described. There's no sense duplicating the struct just to match the number of structs with the number of AI functions.

Introducing unions would also introduce manual management of them, and more complicated code. I think it's not worth it

It's unclear to me what you mean by manual management of unions and how that is somehow a downside to this approach. It seemed like you were on board with the idea of an abstract Monster base class, which is very similar to this approach. Although, I also don't know why you said that it was not feasible.

The only thing that makes it more complicated is the fact that the same variable can have different names in different places in the code. It makes each location where it's used more understandable in isolation, but it's not immediately obvious that monster.goalVars.leoric.moveCounter refers to the same value as monster.goalVars.common.goalVar1. If you are, for instance, trying to figure out how Leoric's goal vars are captured in the save files, you can't just search for usages of moveCounter throughout the codebase.

Perhaps a simple compromise could be to use reference types to alias the variables in the functions themselves. However, this way you lose out on having descriptive variable names at the struct level so it's less clear what the variables are used for unless you explicitly search out their usages.

[mikolaj-pirog]

Thanks for clarification. By manual union management I meant that we would have to make sure we we reading and writing to active field each time we access the union.

I described reworking monster structure as not feasible, since it would be a very big, paradigm shifting rework. If we want to do this, I am on board, since I believe that is the cleanest (and most radical) solution - but it would result in much clearer and manageable code.

I am now more convinced to the union approach - it would solve readability problem and is not as hard to do as OO rewrite.

[heinermann]

The union approach is probably a simpler version of Starcraft's here (replica of official code with different naming). Unions are manually accessed based on unit type or class of unit types (in this case monster type or class of monsters). That code uses an anonymous union to bypass the extra reference. If you adopt the same mechanism then you could do i.e. monster.leoric.moveCounter but maybe you want to keep goalVars.