Clean up M_RoundWalk

[AJenbo]

No description provided.

[StephenCWills]

If we're casting pointers, shouldn't we specify the underlying type of the enum?

[AJenbo]

_mgoalvar2 os one of those nasty temp variables that are used as different things in various contexts. Some times is a relative direciton (left/right) some times it's a cardinal Direciton, some times it's the X component of a position.

[StephenCWills]

Perhaps it was unclear what I was suggesting because of the line that I chose to comment on. Since the type of _mgoalvar2 is int, I think the underlying type of the RotationDirection enum should also be int. From what I'm reading, unless we explicitly specify the type, the standard only specifies that the underlying type will be big enough to represent all its values.

enum RotationDirection : int {
	RotateRight,
	RotateLeft
};

[ephphatha]

Yeah, this is undefined behaviour territory.

Given it's a new enum that isn't used anywhere else I'd really like to use scoped enums (enum class|struct) instead. They default to a backing type of int which avoids this issue.

[ephphatha]

Yeah, this is undefined behaviour territory.

Given it's a new enum that isn't used anywhere else I'd really like to use scoped enums (enum class|struct) instead. They default to a backing type of int which avoids this issue.

[ephphatha]

The only time this updates monster[i]->mgoalvar2 is if the direction checks fail. I think it'd be easier to take this argument by value as that makes the conversion easier and lets it get passed straight through to Rotate().

This does mean updating _mgoalvar2 explicitly as a side effect at the end of the function, ideally that side-effect gets removed in a later refactor.

[AJenbo]

Are you suggesting to take MonsterStruct & instead of RotationDirection * as the second argument?

[ephphatha]

RotationDirection by value as the third argument unless you wanted to unpick the use of the monster struct and array index further down the call stack :)

bool M_RoundWalk(int i, Direction direction, RotationDirection spin) {
    // existing logic updated to not dereference spin
    monster[i]._mgoalvar2 = monster[i].mgoalvar2 == RotationDirection::RotateRight ? RotationDirection::RotateLeft : RotationDirection::RotateRight;
}

(That said I think only DirOk uses the i value in a way that makes refactoring slightly complicated.)

[obligaron]

Can this be further simplified?
The spin parameter is always read from and always written to _mgoalvar2 or?

bool RoundWalk(int i, Direction direction)
{
   RotationDirection spin = static_cast<RotationDirection>(monster[i]._mgoalvar2)
   // existing logic
   monster[i]._mgoalvar2 = static_cast<int>(spin);
}