-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add a spawned player count to the scoreboard #316
base: master
Are you sure you want to change the base?
Conversation
Do you happen to have a screenshot of the scoreboard with this PR? Imagining it, I think I would like a pipe rather than a bullet, but it's difficult to tell for sure without seeing it. (adding support for the bullet would be good anyway though since it could be used elsewhere, if not here) |
Actually yeah, I do like how that looks. It would be a bit smaller of course with the big "Red Faction" logo (which I still think should remain), but it does look nice with the bullet - it draws attention to the two important pieces of information rather than the divider. |
game_patch/hud/multi_scoreboard.cpp
Outdated
}; | ||
const int32_t num_spawned_players = std::ranges::count_if( | ||
SinglyLinkedList{rf::player_list}, | ||
[] (auto& p) { return !rf::player_is_dead(&p)&& !rf::player_is_dying(&p); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space before &&
game_patch/hud/multi_scoreboard.cpp
Outdated
else | ||
game_type_name = rf::strings::team_deathmatch; | ||
rf::gr::string_aligned(rf::gr::ALIGN_CENTER, x_center, cur_y, game_type_name); | ||
const auto get_game_type_name = [] (const rf::NetGameType game_type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make this a standalone function, there's no reason to make it a lambda
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bother? It is just to make up for the fact that if
is not an expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorter functions are more readable and this fragment is very easy to split out
@@ -250,6 +250,7 @@ GrNewFont::GrNewFont(std::string_view name) : | |||
static std::pair<int, int> win_1252_char_ranges[]{ | |||
{0x20, 0x7E}, | |||
{0x8C, 0x8C}, | |||
{0x95, 0x95}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will it work if someone plays with resolution 1024x768 or less? Do we always use TTF font for that header? If not how separator will look like in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it will not render for the small hud I think. But does it really matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I sometimes use that resolution myself to test things in window. It can't crash, that's obvious. But it would be nice to also look good. If it looks okayish I guess we can leave it like this. Ultimately if we want to introduce more characters in the future (e.g. unicode?) we may want to fully stop using builtin fonts, then the problem will solve itself
@@ -88,6 +92,16 @@ namespace rf | |||
NG_TYPE_TEAMDM = 2, | |||
}; | |||
|
|||
inline std::string_view multi_game_type_name(const NetGameType game_type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you put those functions (multi_game_type_name
and multi_num_spawned_players
) into rf
directory? This directory is supposed to only contain addresses/type definitions for RF. Please move them to multi_scoreboard.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rf
should be like an sdk or library e.g. if rf
were a crate it would contain these small and simple functions. These functions should be expected in rf
from a developer's point of view because it is intuitive and consistent. It also works well with intellisense. They do not properly belong to the implicit namespace dash
.
@@ -151,6 +165,10 @@ namespace rf | |||
static auto& multi_tdm_get_red_team_score = addr_as_ref<int()>(0x004828F0); // returns ubyte in vanilla game | |||
static auto& multi_tdm_get_blue_team_score = addr_as_ref<int()>(0x00482900); // returns ubyte in vanilla game | |||
static auto& multi_num_players = addr_as_ref<int()>(0x00484830); | |||
[[nodiscard]] inline int32_t multi_num_spawned_players() { | |||
const auto f = [] (auto& p) { return !player_is_dead(&p) && !player_is_dying(&p); }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f
is not very descriptive, maybe is_alive
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not want to split this line. It works better in a single line imo.
@@ -151,6 +165,10 @@ namespace rf | |||
static auto& multi_tdm_get_red_team_score = addr_as_ref<int()>(0x004828F0); // returns ubyte in vanilla game | |||
static auto& multi_tdm_get_blue_team_score = addr_as_ref<int()>(0x00482900); // returns ubyte in vanilla game | |||
static auto& multi_num_players = addr_as_ref<int()>(0x00484830); | |||
[[nodiscard]] inline int32_t multi_num_spawned_players() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a name multi_num_alive_players
, because alive is opposite to dead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True but no one speaks like that. It is not idiomatic.
Based off #274.
Displays
CAPTURE THE FLAG • 2/3 PLAYING
.•
is now an allowed glyph because it is useful for UI. It did not exist in RF's fonts.Imo it is nice to be able to glance at the number of players spawned or the number of players joined.
Requires #314 to compile.