Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Converted the player palette to an enum class #303

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

Pheazant
Copy link
Contributor

No description provided.

Copy link
Owner

@mmatyas mmatyas left a comment

Choose a reason for hiding this comment

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

Nice, thank you! Just 2 minor things, otherwise looks good.

src/smw/player.h Outdated
@@ -90,6 +90,7 @@ class CPlayer
short getGlobalID() const { return globalID; }
short getTeamID() const { return teamID; }
short getColorID() const { return colorID; }
PlayerPalette getPlayerPalette();
Copy link
Owner

Choose a reason for hiding this comment

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

We can make this const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

player.animationstate += 32;
if (player.animationstate > 96)
player.animationstate++;
if (player.animationstate > 3)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can use the new enum values here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Owner

@mmatyas mmatyas left a comment

Choose a reason for hiding this comment

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

I really like that you managed to remove the separate animation fields. There are some readability questions about the implementation.

return invincibility_animation_states[(timer / 4) % 4];
} else {
return invincibility_animation_states[((timer - 4) / 7) % 4];
}
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any way we could make the indices more readable, that would explain why these numbers are used? for example,

  • If you use std::array for the animation states, you can then use its size() in the % 4 part
  • The / 4 and / 7 parts seem to control the speed of the animation – you could move them to a named variable
  • Why do we subtract - 4 in the second branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Copy link
Owner

@mmatyas mmatyas left a comment

Choose a reason for hiding this comment

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

Ok, this looks good, thanks!

@mmatyas mmatyas merged commit 721bb23 into mmatyas:master Apr 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants