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

Feat: settime chat command #702

Merged
merged 24 commits into from
Aug 25, 2024
Merged

Feat: settime chat command #702

merged 24 commits into from
Aug 25, 2024

Conversation

ToeKneeRED
Copy link
Contributor

@ToeKneeRED ToeKneeRED commented Aug 15, 2024

It is about the same if not the same as the previous PR for it from cosi.
Adds a /settime chat command that can be used on any server that is not public.

Ideally, there would be some feedback from the server that it succeeded or failed besides the time change (e.g. a localized chat message, but that turned out to be too complex for me).

EDIT:
Since admins should be the only ones allowed to use the /settime chat command, there are now two different ways of becoming an admin; either by joining the server using the admin password specified in the STServer.ini file, or using the new /AddAdmin console command.
Unfortunately, spaces in names seem to be nearly impossible to deal with without replacing the characters and re-checking for the name over and over again. For the sake of this PR, you can't /AddAdmin or /RemoveAdmin on a name with an underscore in it as that is what is used to replace since spaces are more likely to be in names rather than underscores.

Three new console commands:

/AddAdmin : add the player with the username given to the adminSessions Set
ex.
/AddAdmin Toe_Knee = Toe Knee
/AddAdmin ToeKnee = ToeKnee
/RemoveAdmin : remove the player with the username given from the adminSessions Set
/admins: list out all admins

The PR also now includes a fix for a crash with the ConsoleRegistry's manual resize once it reaches a size() of 10 (it tried to erase m_commandHistory.end()).

Properly sets time for connected players, but with no system chat message
Properly sets time for connected players, but with no system chat message
* tweak: Add Services, World, and GameServer bindings

GameServer_Bindings:
    - SendGlobalChatMessage
    - SetTime
Player_Bindings:
    - IsPartyLeader
Service_Bindings:
    - "get" for all services
    - CalendarService : GetTimeScale
    - PartyService  : IsPlayerInParty
World_Bindings:
    - "get"

* tweak: bool return for GameServer SetTime

* tweak: Add onSetTime scripting event

* tweak: Set up for settime chat command

* tweak: different .cpp binding file for each service
- /AddAdmin: add the player with the username given to the adminSessions Set
- /RemoveAdmin: remove the player with the username given from the adminSessions Set
- /admins: list out all admins
also includes a fix for crashing after 10 commands are sent to the server
Properly sets time for connected players, but with no system chat message
- /AddAdmin: add the player with the username given to the adminSessions Set
- /RemoveAdmin: remove the player with the username given from the adminSessions Set
- /admins: list out all admins
also includes a fix for crashing after 10 commands are sent to the server
@ToeKneeRED
Copy link
Contributor Author

Since admins should be the only ones allowed to use the /settime chat command, there are now two different ways of becoming an admin; either by joining the server using the admin password specified in the STServer.ini file, or using the new /AddAdmin console command.
Unfortunately, spaces in names seem to be nearly impossible to deal with without replacing the characters and re-checking for the name over and over again. For the sake of this PR, you can't /AddAdmin or /RemoveAdmin on a name with an underscore in it as that is what is used to replace since spaces are more likely to be in names rather than underscores.

Three new console commands:

  • /AddAdmin <username>: add the player with the username given to the adminSessions Set
    ex.
    /AddAdmin Toe_Knee = Toe Knee
    /AddAdmin ToeKnee = ToeKnee
  • /RemoveAdmin <username>: remove the player with the username given from the adminSessions Set
  • /admins: list out all admins

The PR also now includes a fix for a crash with the ConsoleRegistry's manual resize once it reaches a size() of 10 (it tried to erase m_commandHistory.end()).

Copy link
Member

@Force67 Force67 left a comment

Choose a reason for hiding this comment

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

Also please run clang format python script (tools/scripts/clang_format.py) over your changes.

@@ -383,6 +391,129 @@ void GameServer::BindServerCommands()
out->error("Day must be between 0 and 31, month must be between 0 and 11, and year must be between 0 and 999.");
}
});

m_commands.RegisterCommand<std::string>(
"AddAdmin", "Add admin privileges from player", [&](Console::ArgStack& aStack) {
Copy link
Member

Choose a reason for hiding this comment

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

to player

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

if (!playerFound)
{
// space in username handling
String backupUsername = cUsername;
Copy link
Member

Choose a reason for hiding this comment

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

Should factor out the user name sanitizing / cleaning into a separate utility fn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with GameServer::SanitizeUsername (currently only replaces any _ with space if name is not found the first time through)


for (const auto& cAdmin : m_adminSessions)
{
Player* pPlayer = PlayerManager::Get()->GetByConnectionId(cAdmin);
Copy link
Member

Choose a reason for hiding this comment

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

Probably the get user by name stuff too. maybe have a fn there to get a user by name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added two things for this, GameServer::GetAdminByUsername and PlayerManager::GetByUsername
Reworked that part to adapt it in as well

for (const auto& cAdmin : m_adminSessions)
{
const auto& cUsername = PlayerManager::Get()->GetByConnectionId(cAdmin)->GetUsername();

Copy link
Member

Choose a reason for hiding this comment

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

Even though unlikely, we should still handle if GetConnectionId somehow returns null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now has a nullptr check and prints to console if m_adminSessions contains a session it shouldn't


protected:
// Event handlers
void HandleUpdate(const UpdateEvent& acEvent) noexcept;
void HandleConnect(const ConnectedEvent& acEvent) noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

We should for safety reasons tombstone the localplayerid on disconnect. e.g. set it to invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with new HandleDisconnected

@@ -747,8 +879,14 @@ void GameServer::HandleAuthenticationRequest(const ConnectionId_t aConnectionId,
}

// check if the proper server password was supplied.
if (acRequest->Token == sPassword.value())
if (acRequest->Token == sPassword.value() || (acRequest->Token == sAdminPassword.value() && !sAdminPassword.empty()))
Copy link
Member

Choose a reason for hiding this comment

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

might be nicer to assign the result of the second expr to a boolean then we can also use the result below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with additional bool above

{
m_isPublic = aIsPublic;
}
bool IsPublic() const noexcept
Copy link
Member

Choose a reason for hiding this comment

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

nodiscard

@@ -78,6 +78,14 @@ struct GameServer final : Server
{
return m_isPasswordProtected;
}
void SetPublic(bool aIsPublic) noexcept
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for this. the value if a server is "public" is contained by bAnnounceServer convar. if you need it from another cpp file, you can extern Convar name it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it, and the reference to it that was in ServerListService

formatted all files except those that were in the scripting expansion
@ToeKneeRED
Copy link
Contributor Author

clang-format'd all files except those in the scripting expansion because I accidentally rebased instead of merge and it wants to re-add those files

@Force67 Force67 merged commit 6adb5d0 into tiltedphoques:dev Aug 25, 2024
0 of 2 checks passed
@ToeKneeRED ToeKneeRED deleted the feat/setTime branch August 25, 2024 15:30
@rfortier
Copy link
Contributor

I'm late to the party since this was just merged. But I added it to the latest Modded Animation pre-release binary build, after verifying (shocker, it just works). So we can get a few more testers hands-on before a Master release.

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.

3 participants