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

Refactoring of existing cheats (see #1679) #1688

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kubaau
Copy link
Contributor

@kubaau kubaau commented Jul 28, 2024

First few commits from PR #1679 :

  • refactors existing cheat code ("winter" code, toggle human AI player, armageddon) into new Cheats and CheatCommandTracker classes

Copy link
Member

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

Great use of the circular buffer, good idea!

It does make sense to separate the cheats and the key/command tracker although I don't really like putting the cheats into the world class or having the command tracker be a part of the cheats creating a cyclic dependency and having to do cheats.trackX -> tracker.trackX

So maybe add an instance of each class to the game interface?

libs/s25main/world/GameWorldBase.h Show resolved Hide resolved
libs/s25main/CheatCommandTracker.cpp Outdated Show resolved Hide resolved
libs/s25main/CheatCommandTracker.cpp Outdated Show resolved Hide resolved

bool CheatCommandTracker::trackSpecialKeyEvent(const KeyEvent& ke)
{
if(ke.kt == KeyType::Char)
Copy link
Member

Choose a reason for hiding this comment

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

I guess it makes more sense to move this check to the caller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep this here. This logic gets (slightly) more complex when other functions get involved, like in the bigger PR @ https://github.com/Return-To-The-Roots/s25client/pull/1679/files#diff-d98c107f14a70f47245ba076ec4e845508c123b64120cd47c4b8e6f9f14b4384R21 . It also clearly shows that this function does nothing for char keytypes.

Copy link
Member

Choose a reason for hiding this comment

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

I see. So the idea for those functions is to return true if they handled the event and if not the next function gets a shot.

So maybe checkSpecialKeyEvent is better. I was confused about calling "specialKeyEvent" and "charEvent" with the same event which looked odd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rename track to check or something similar. "Track" is a byproduct of refactoring that does not explain much.

libs/s25main/CheatCommandTracker.cpp Outdated Show resolved Hide resolved
libs/s25main/Cheats.h Outdated Show resolved Hide resolved
libs/s25main/Cheats.h Outdated Show resolved Hide resolved
@@ -409,7 +409,7 @@ void dskGameInterface::Msg_PaintAfter()
DrawPoint iconPos(VIDEODRIVER.GetRenderSize().x - 56, 32);

// Draw cheating indicator icon (WINTER)
if(isCheatModeOn)
if(world.IsCheatModeOn())
Copy link
Member

Choose a reason for hiding this comment

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

IMO cheats are not part of the world but rather belong to the interface. Or is there a reason for moving them down? I'd make an instance of Cheats in this class instead which also avoids going through game->world_.GetCheats and the inconsistent access here, i.e. why isn't this using GetCheats?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree completely. In principle it does not make much sense for Cheats to be in World. The problem is that if you look at the cheats in the other PR most of them can easily be implemented by going through World. Also a lot of cheat implementations only need to know if IsCheatModeOn() and do not need the internals that GetCheats() gives. This avoids doing GetCheats().IsCheatModeOn().

Moving Cheats to GameInterface makes intuitive sense, but some of the classes that use Cheats know nothing about GameInterface. I think they would at least need GetGameInterface to be part of GameWorldBase and then any calls to cheats would need to go through world().getgi().dosomething(). To avoid this would require passing GameInterface all over the place.

Also a case can be made for cheats to be outside of the GameInterface: for example in S2 the cheat activation state is preserved through games. To achieve this (if desired) Cheats would have to be in some more global state.

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 nothing wrong with cheats modifying the world. They also don't need the gameinterface as that would be backwards: The GameInterface has a cheats and world. The cheats use the world (modify it)

So my idea was to pass the world into the cheats class (controller->model pattern) where needed.

But I see you need the cheats in the GameWorldViewer. Maybe they are better of there. That could break the cycle: GWV has cheats and world-reference, cheats has world-reference. That could even be just a reference in GWV where dskGameInterface has the actual class (and possibly the handlers/tracker)

Actually I caused some confusion here: The interface is the desktop, not the GameInterface class itself. I guess that would work then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay. I'll take a look at this (and other PR's) shortly.

libs/s25main/desktops/dskGameInterface.cpp Show resolved Hide resolved
libs/s25main/Cheats.cpp Outdated Show resolved Hide resolved
- avoid early returns
- rename cheatStr to enableCheatsStr
- rename canCheatModeBeOn() to areCheatsAllowed()
- trackCharKeyEvent return void
- hold cheatCmdTracker_ by value instead of unique_ptr
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