Prophet now needs to complete tasks to earn revelations - Fixes #190 #206
Conversation
| var taskInfos = __instance.Data.Tasks.ToArray(); | ||
| var allTasksCount = taskInfos.Count; | ||
| var maxRevealsCount = | ||
| CustomGameOptions.ProphetTotalReveals < 1 ? 1 : CustomGameOptions.ProphetTotalReveals; |
There was a problem hiding this comment.
Isn't ProphetTotalReveals bounded to be a minimum of 1?
There was a problem hiding this comment.
being too defensive. rewritten to remove divison eliminating both risk of float imprecision and divide-by-zero.
| var allTasksCount = taskInfos.Count; | ||
| var maxRevealsCount = | ||
| CustomGameOptions.ProphetTotalReveals < 1 ? 1 : CustomGameOptions.ProphetTotalReveals; | ||
| var currentRevealsCount = Role.GetRole<Prophet>(PlayerControl.LocalPlayer).Revealed.Count; |
There was a problem hiding this comment.
This is using PlayerControl.LocalPlayer but the method takes in a PlayerControl __instance object. Not sure which is intended.
There was a problem hiding this comment.
I modelled it after CompleteTask.cs from the Snitch, where it seems like both patterns are used. I think they both work, but not sure what the implications are. I tried a bunch of different multi-client setups to test and didn't run into any problems as coded.
There was a problem hiding this comment.
Also just removed the __instance and it still works. It seems like both PlayerControl.LocalPlayer and __instance are really the same thing.
| var timeSpan = now - role.LastRevealed; | ||
| var cooldown = CustomGameOptions.ProphetCooldown * 1000f; | ||
| var requiredTasksForNextReveal = (currentRevealsCount + 1) | ||
| * ((float) allTasksCount / maxRevealsCount); |
There was a problem hiding this comment.
Is this handling things as float? So if we have 8 tasks and 3 reveals, it will trigger at 3, 6 and 8?
Is there a possibility of floating point imprecision accidentally denying someone the last reveal?
There was a problem hiding this comment.
wouldn't really happen at the scale of these numbers, but, rewritten to remove divison eliminating both risk of float imprecision and divide-by-zero.
There was a problem hiding this comment.
So if we have 8 tasks and 3 reveals, it will trigger at 3, 6 and 8?
that is correct.
…ion and divide-by-zero errors.
|
Looks good to me and it held up to some simple testing. Would like to try it out some night with more people before releasing to the general public. |
You only get as many revelations as tasks. The prophet is a strong role as it is and having more revelations than tasks is potentially too strong. Revelations are doled out as (tasks / reveals) number of tasks (rounded up) are completed.