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.

Sign up for GitHub

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

Jump to bottom

Enhances Roundstart Antagonist Selection to Prevent Roundstart Fail and Allocate All Antags #1339

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

Enhances Roundstart Antagonist Selection to Prevent Roundstart Fail and Allocate All Antags #1339

wants to merge 12 commits into from
+107 −14

Conversation

rbertoche
Copy link
Contributor

@rbertoche rbertoche commented Dec 12, 2024 •
edited
Loading

We might never see the end of new antags being added using MakeAntag so better just cut it off by the root and mitigate the issue by preventing it from causing the round not to start.

Description

Since this issue still persisted to manifest even after an attempt to fix it, this PR simply prevents the rounds from not starting due to the error by changing the exception into a log error.

Catching and treating could be done instead, but I don't see many benefits from that at all, so an error it is.

Also important, after asking a player in ahelp about what settings they had, I concluded it gets triggered when a player has Thief antagonist enabled, and have jobs set up in such a way that they remain in lobby even when they ready up. So they seem "elligible" but they have no entity at roundstart due to their setting to not get spawned at roundstart as assistant and wait in lobby when you don't have the jobs you had selected.

I tried to figure a way to still add the same antagonist it failed to assign in ChooseAntags, and considered decrementing i inside that loop, but then I didn't due to how decrementing i would make the loop prone to stalling. So we lose one of the antagonist slots, but get the round to start, best effort.

Cheers.

Edit: Please read my last comments.

TODO

Media

Example Media Embed

Changelog

🆑

  • tweak: Enhances roundstart antagonist selection making sure all slots are allocated
  • fix: Prevents Thief or others antags from causing the round to fail to start

@rbertoche
Switches off throw in MakeAntag to a log.Error to prevent further errors
0df6205 
We might never see the end of new antags being added using MakeAntag so
better just cut it off by the root and mitigating the issue by
preventing it from causing the round to not start.
@github-actions github-actions bot added Changes: C# Changes any cs files Changes: YML Changes any yml files labels Dec 12, 2024
@SimpleStation14 SimpleStation14 changed the title Switches off throw in MakeAntag to a log.Error to prevent further errors Switches Off Throw in MakeAntag to a log.Error to Prevent Further Errors Dec 12, 2024
@rbertoche rbertoche changed the title Switches Off Throw in MakeAntag to a log.Error to Prevent Further Errors Switches Throw in MakeAntag to a log.Error to Prevent Further Errors Dec 12, 2024
@Remuchi
Copy link
Contributor

Remuchi commented Dec 12, 2024

Current implementation can lead to no antags being selected in the round at all. IMO if this error happens, the antag selection should reselect player for that role

@rbertoche
Copy link
Contributor Author

Agreed, but given the reason why it fails to assign I didn't figure a way to retry to assign the role without risking freezing on that loop on the case every candidate is not going to pass the checks.
I would do a change in that direction otherwise

@rbertoche
Copy link
Contributor Author

Well.
I can do a silly hack and like retry 5 times. That would retry, and would not risk a forever stalling loop. Is that a better change?

@rbertoche
Copy link
Contributor Author

Set a constant to determine the max number of attempts. Retry until that number or until the required slots are assigned. It wouldn't be too bad. I can do that later tonight.

sleepyyapril
sleepyyapril requested changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@sleepyyapril sleepyyapril left a comment

Choose a reason for hiding this comment

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

Make it return a bool. If it fails, try try again up to a constant number of tries.

@sleepyyapril sleepyyapril added Priority: 4-Low Should be resolved at some point Size: 5-Very Small For especially small issues/PRs labels Dec 13, 2024
@rbertoche
Copy link
Contributor Author

Would changing the signature of MakeAntag for this be worth it? It would make EE antag code less compatabol.
A new method on the other hand would have less potential of requiring conflicts.
For one I could do a new method, keep the exception and intercept it... But it would have the downside of not working properly for code which just calls MakeAntag. I am not even sure other code calls it... Maybe I am worrying for nothing.
But right now I would think making a retry count inside ChooseAntags would be the less impactful change which still would fix not picking enough antags.
But if you insist I will just make it return bool sure.

@sleepyyapril
Copy link
Contributor

Would changing the signature of MakeAntag for this be worth it? It would make EE antag code less compatabol. A new method on the other hand would have less potential of requiring conflicts. For one I could do a new method, keep the exception and intercept it... But it would have the downside of not working properly for code which just calls MakeAntag. I am not even sure other code calls it... Maybe I am worrying for nothing. But right now I would think making a retry count inside ChooseAntags would be the less impactful change which still would fix not picking enough antags. But if you insist I will just make it return bool sure.

Mm, true, should be fine as is.

@rbertoche
Copy link
Contributor Author

rbertoche commented Dec 22, 2024 •
edited
Loading

So. I ended up changing MakeAntag a lot while preserving APIs. But now I need to properly test this, since it's not tiny anymore.
I think previous code was not great as a consequence of the lack of features on AntagSelectionPlayerPool - or maybe it shouldn't have become a class at all and having to duplicate Random and List methods is a code smell? Anyway
The added methods make it way easier to write proper code to get N antags, and to filter the pool in a sane way.

@rbertoche
Copy link
Contributor Author

rbertoche commented Dec 22, 2024 •
edited
Loading

There's only one thing which I'm not sure about these last changes. Calling MakeAntag(ent, null, def) to fill up unused slots makes sense, but it may have unintended consequences.
What is better? Just leave some antags missing if count does not get to 0 due to hitting maxRetries, or call MakeAntag(ent, null, def) to fill these slots? Current code does the latter.

@rbertoche rbertoche changed the title Switches Throw in MakeAntag to a log.Error to Prevent Further Errors Enhances roundstart antagonist selection to prevent roundstart fail and allocate all antags Dec 22, 2024
@SimpleStation14 SimpleStation14 changed the title Enhances roundstart antagonist selection to prevent roundstart fail and allocate all antags Enhances Roundstart Antagonist Selection to Prevent Roundstart Fail and Allocate All Antags Dec 22, 2024
@rbertoche
Copy link
Contributor Author

rbertoche commented Dec 22, 2024 •
edited
Loading

Instead of writing a quick hack I tried to make ChooseAntags better in a non-hacky way.

@rbertoche
Copy link
Contributor Author

Tested it briefly with a single connection. It did add Thief at roundstart with no issues and didn't break when there weren't any thief available at roundstart either.

@rbertoche
Replicates upstream change which is going to be necessary
622a017 
Made this portion of the code identical to wizden so to not conflict
@rbertoche
Copy link
Contributor Author

rbertoche commented Dec 22, 2024 •
edited
Loading

https://github.com/Simple-Station/Einstein-Engines/actions/runs/12453149394/job/34762931010#step:10:362

This looks relevant but I am not sure how to fix it. It's the Log.Error in the changes.

@rbertoche
Copy link
Contributor Author

I think this error is supposed to happen in the tests. This new ChooseAntags will detect the failure and try to assign the roles, so I could change it from an error to a info or at least a warning. But since I already copied the state of that code region from wizden I don't think it's worth it to change it to reduce the log level.

Feel free to fix the test as you think is appropriate and merge if you want, if it's convenient

@rbertoche rbertoche mentioned this pull request Dec 22, 2024
Closed
2 tasks
@sleepyyapril
Copy link
Contributor

https://github.com/Simple-Station/Einstein-Engines/actions/runs/12453149394/job/34762931010#step:10:362

This looks relevant but I am not sure how to fix it. It's the Log.Error in the changes.

Could check if the player is valid, like IsTerminatingOrDeleted, and if not choose another.

@rbertoche
Copy link
Contributor Author

rbertoche commented Dec 23, 2024 •
edited
Loading

Right, after you say it it seems so obvious :P
I assumed there weren't enough candidates in the test, I'm gonna try that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sleepyyapril sleepyyapril sleepyyapril requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Changes: C# Changes any cs files Changes: YML Changes any yml files Priority: 4-Low Should be resolved at some point Size: 5-Very Small For especially small issues/PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rbertoche @Remuchi @sleepyyapril