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

Implement 54d8a38 even properlier #1604

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Implement 54d8a38 even properlier #1604

wants to merge 1 commit into from

Conversation

sprunk
Copy link
Collaborator

@sprunk sprunk commented Jul 18, 2024

Let people keep the debug airmesh view if they disable cheats after enabling /airmesh, since now they are forced to keep cheats enabled for the whole game even if they only wanted it for that one command.

@sprunk sprunk requested a review from lhog July 18, 2024 15:27
@lhog
Copy link
Collaborator

lhog commented Jul 18, 2024

I don't know about this one. I have hard time imagining this practically:

  1. Someone wants to debug airmesh
  2. They enable cheating
  3. They enable /airmesh
    .....
  4. They disable /cheats.... why?

@sprunk
Copy link
Collaborator Author

sprunk commented Jul 18, 2024

If you watch a replay then you want to disable cheats ASAP because game logic may work differently if you keep them on -> desync.

@sprunk
Copy link
Collaborator Author

sprunk commented Jul 18, 2024

Maybe the better approach would be to allow the command in replays and/or spectators?

@lostsquirrel1
Copy link
Collaborator

Isn't there a process for this stuff already? I recall debugpath can be used freely by spectators wihtout cheats, but players have to enable cheats. I can't recall whether cheats need to be enabled in replay for it to work.

@sprunk
Copy link
Collaborator Author

sprunk commented Jul 19, 2024

Yes, /airmesh used to work that way too. The issue was that the automated documentation generator only looks at the "only togglable with cheats?" bool, which was not set (to allow spectators to call it, or to let players disable it) and does not know about any further checks. So players could not use /airmesh without cheats and were confused because the docs did not say anything about requiring cheats.

@lhog
Copy link
Collaborator

lhog commented Jul 19, 2024

If you watch a replay then you want to disable cheats ASAP because game logic may work differently if you keep them on -> desync.

/cheats alone is not game intrusive.
I think if someone to do /cheats, /airmesh, /cheats will immediately understand why the smooth mesh display is gone, just by the cause and effect sequence.

Maybe the better approach would be to allow the command in replays and/or spectators?

Makes sense, class IActionExecutor needs to be extended then to handle such commands in a uniform way.

@sprunk
Copy link
Collaborator Author

sprunk commented Jul 19, 2024

/cheats alone is not game intrusive.

Yes, but a game can have gadgets with if Spring.IsCheatingEnabled() then.

@lhog
Copy link
Collaborator

lhog commented Jul 21, 2024

/cheats alone is not game intrusive.

Yes, but a game can have gadgets with if Spring.IsCheatingEnabled() then.

Well in that case the sync state is busted anyway as you can't time events like cheats on and off and rely on Spring.IsCheatingEnabled() to manipulate the state.

@sprunk
Copy link
Collaborator Author

sprunk commented Jul 21, 2024

Fair enough. In that case how about keeping the cheat check but also letting spectators view it? Like is done in path debug as suggested by @lostsquirrel1:

if (!gs->cheatEnabled && !gu->spectating)

- if (!(drawEnabled && gs->cheatEnabled))
+ const bool allowDraw = gs->cheatEnabled || gu->spectating;
+ if (!drawEnabled || !allowDraw)

@lhog
Copy link
Collaborator

lhog commented Jul 21, 2024

Yeah, we should extend class IActionExecutor to add the spectator role as an option or add a functor to check for "enabled" conditions. I'll have a look into this one.

@sprunk sprunk marked this pull request as draft September 18, 2024 13:59
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