Fix bugs with click limiter and add configurable semi-auto CPS limit to dedicated server config #270
Conversation
…fixed 2 bugs that resulted in dropped fire packets
|
I really dislike pistol and precision rifle spam so this PR is great. But I have a couple thoughts:
|
I disagree. Even if the current terminology is not fully descriptive to how the feature technically works, it's far easier for the average player/server operator (especially one who has played the game for some time) to understand - which is to an enormous degree more important. Breaking it into parts: "$DF Semi Auto Click Limit"
I suppose something like "$DF Semi Auto Click Rate Limit" could be used instead, but that really feels like splitting hairs if I'm honest, and keeping the labels short is generally positive.
I agree. That said, I don't know off the top of my head the best approach to take on that aspect while avoiding unintended negative consequences. I suppose it would just need these elements:
What worries me though, is potential discrepancies in timestamps tracked by the client and the server might realistically result in clicks being registered by a client but dropped by a server, or vice versa. It would be simple if this could just be handled by the client, but that's not an option here, both because we can't assume 100% of players will have updated clients, and for general cheat prevention reasons. It's also probably worth considering that an attempt to limit the client's fire rate like this is almost certainly to put players with upgraded clients at a disadvantage compared to other players, if they're legitimately clicking quick enough for it to be relevant. If they're using an autoclicker, I don't really care if what they see reflects what the server knows (honestly it'd probably be better if it didn't, since doing so would just be telling people who are trying to cheat the maximum rate they can get away with). IMO the best approach as it sits is to add the server side part of this (ie. this PR) and defer determining if/how and implementing (if necessary) the approach on clients to a separate feature/enhancement down the road. |
This PR does not add this. Server-side fire rate limit is already part of last DF version. But still from what I heard it isn't working that great because the limit is set high and limit is set high because some players click fast lol
It's not interval (duration between shoots) but number of shoots per second.
Client-side anti-cheats can always be hacked so it's always best to have server-side anti-cheats where possible. It doesn't mean that DF can't do more on the client-side. AC should have multiple layers of protection, one of them being the client-side AC and the second a server-side AC. |
|
For reference, this video demonstrates each of the issues I referenced above - dropped fire requests under the following circumstances:
In each case, you can see the drops logged in the server console, and when I reload the weapons, from the ammo removed, you can see shots that didn't register. Also with the rocket, the dropped rockets don't create geo craters. |
…culated based on the minimum fire wait between primary/alt fire. This implementation is less able to be abused and more mod-friendly.
It checks if a fire interval average converted to clicks per second is less than
Let me clarify. A client should not let a weapon be fired if it knows it is going to be blocked by a server. It is easy to implement and it may as well be part of this pull request.
Is it not easier to understand a shortest fire interval of 66 ms instead of a maximum clicks per second of 15? This implementation does not use a counter so it does not properly allow burst firing. It has to be manually converted to shortest fire interval in your head.
How?
How? |
While the current approach isn't ideal for this reason, I still think it's most practical. Especially given there is no realistic way to ensure client behaviour matches, the current approach avoids unintuitive cases where, for example, a player double clicks very quickly but only one of those clicks actually registers on the server, even though both appeared visually on their client. In that example, they would also in effect be forced to reload after firing only 19 shots (from the perspective of the server). The idea with the configurable click limiter in my mind is far more to mitigate the impact of autoclickers rather than regulate all clicking with semi auto weapons to ensure it's at a "reasonable" rate (if that was the aim, it'd be better to just force them to behave like automatic weapons like many TC mods do).
Honestly, no. In most cases a person would probably understand both, but given how common it is to measure weapon effectiveness in DPS (damage per second) in games, CPS is, I think, more simple for the average person to wrap their head around. Furthermore, measuring semi auto fire rate in CPS has been historically popular in RF since the beginning, so it's almost certainly a more coherent way to communicate the behaviour to RF players in particular.
I would normally agree with this statement, but for the reasons below, it's not near as simple as it seems.
It is easy to implement in theory, but not near as easy to reasonably handle all cases in practice. I consider it room for future expansion on the idea, but strictly outside of scope for this PR.
The updated client would likely fire fewer shots in practice because it is attempting to throttle itself based on its understanding of whether it can fire on the server. This, in theory, could be even more problematic if the client is clicking quicker than their latency or suffering any amount of packet loss, etc. A non-updated client continues to send the server fire requests at the rate the player is clicking, and even though many of their requests are dropped, they're not being throttled in the amount of fire requests they can send to the server, meaning they're far more likely to have their clicks converted into fire requests. In addition, a player legitimately clicking slightly in excess of the cps limit is likely to have some of their clicks never converted into fire requests, whereas someone using an autoclicker would, in a scenario where the client enforced the cps limit as well, enjoy a scenario where they click at exactly the maximum allowable rate and never waste ammo by sending fire requests that are rejected - in such a case, it would be a bit silly to not use an autoclicker (with an updated client), since doing so would just give you effectively perfect behaviour. Again, I'm not saying the clientside portion isn't worth exploring - it is - it's just a relatively big and complex undertaking that must be approached delicately and carefully... and it's far less important than the serverside portion in general. Being practical, I consider the clientside portion out of scope for this PR, which does as-is effectively solve the immediate problem and provides an experience that is vastly superior to the current state. |
Can this scenario not happen with the current implementation? I think each shot's interval should be checked individually for consistency. The current implementation does not reset after a player's death as well.
I do not think this analogy is correct. DPS and CPS are very different. This PR is going to block shots below a certain interval rate so it is going to be relevant.
I see but packet loss is rare and it works similar for other weapons. On that note we do need metrics for packet loss and latency.
Was preventing an autoclicker's very fast clicking not this PR's purpose? A client should know a server's CPS so an autoclicker can be used in any case. Besides can you not brute force a server's CPS by seeing how sound varies at various click rates with a pistol or calculating ammo discrepancies?
Why? Is it not just checking a timer upon firing and conditionally blocking or signaling?
I do not think it is acceptable for this PR to allow firing to be silently blocked. Edit: I know this PR is just additions to a click limiter but it will be more accessible and I think it is flawed. There should be some kind of client-side signal for firing too fast like a sound or visual indicator if shots were not going to be blocked client-side. |
I think it's unlikely with the averaging between the 4 samples (unless you're clicking fast prior to the double click, I guess). The situation I described would happen in a situation where every bullet was checked to see if a timer had elapsed and dropped if not.
That's true, and it could be an opportunity for improvement on the PR. That said, I'm not sure in practice it is likely to ever matter given the time it would take to die, wait for the death animation to play, respawn, and start shooting again.
True, but my point is that players are very used to measuring things over a second. CPS is much easier, I think, for players (of both RF and other games) to wrap their heads around, as opposed to specifying a minimum ms delay or something like that.
Yes, that is the PR's purpose (well, plus the bug fixes). There's no reason for the client to need to know the server's CPS unless there is a clientside piece of this (which currently there is not). Sure you could figure it out though, I'm not saying its super secret hidden info or anything, I'm just saying that replicating this click limiter functionality on the client side as you're suggesting would disadvantage normal users who have upgraded clients (when compared to folks using older clients), and also make using an autoclicker to click at the perfect rate (which would be impossible manually) enticing.
Technically yes, but as I've described above, there are other things that need to be considered in implementing it to avoid adverse effects, which I think are highly likely with the implementation you're describing. It's not near as simple as just writing the functionality to replicate the existing behaviour on the client. Figuring out and implementing the best approach to the clientside piece is a much more exhaustive process than I think makes sense for this PR. This is an incremental improvement. Given the CPS limiting functionality for semi auto weapons already exists, this PR is simply allowing server operators control over the allowed rate, I see absolutely no reason to hold up this incremental improvement, which is a mitigation measure for a current problem, in search of a perfect solution to a related problem.
I think this would be more confusing than anything, and not the right way to go about it. I'd much prefer to keep things simple for now and improve it over time. |
A client should know so that they may change how fast they fire. A server operator knows and they may play as well. Further they can tell other people their server's CPS which may be low. Hence a group of people can have an advantage over others because a shot can be silently dropped by a server and you cannot know how many times it happened. |
|
I agree that the client should receive some kind of feedback when their shots are dropped and I don't think this functionality should be merged until that's figured out. I'm less concerned with the potential for a server operator and their friends to keep the interval hidden info for an advantage (although if we rewind 10-15 years, this would definitely have been problematic), but the player should always be able to trust what they experience in-game to the greatest extent possible. The game already has a poor player experience in terms of the discrepancy between client and server with weapon spread, blood and sound on impacts, and whatever else. We should not add to this confusion for the sake of deterring cheating. As far as a sound or visual feedback goes, I think there's already a sort of de-facto method for the server to inform the player/client of stuff. Just send a chat message from the server to the client every time a shot is dropped. RF players are already well accustomed to reading stuff in the chat box. Yes, this would allow players to figure out the interval and use an auto-clicker to fire at a "perfect" rate. Server operators should take this into consideration with their rate limits and configure the highest they would tolerate. People using an autoclicker to fire at a perfect interval is a scenario that is a complete non-issue in comparison to the status quo. Yes, in some ways using an autoclicker to change these guns from semi-auto to full auto (and thus from 'click-timing' to 'tracking' in terms of aiming style) affects the skill required to use the weapon one way or another depending on the player. But this is already the case now with the current behavior, and there's no practical solution to this that doesn't involve breaking compatibility with older clients and a ton of work (not just in code or testing, but politics) to implement. I expect autoclicker use would become more common after this PR, so to anticipate this somewhat, it's worth discussing whether an auto-fire should just be implemented in the client. It could match the server's fire rate limit (up to a sane limit, like 7-10). Without this, there's a strong chance that we end up in a future situation where a lot of experienced players are using auto-clickers at the server's max interval, while new players have no knowledge of this. But even this situation is far better than what we've collectively tolerated for 20+ years. I also think that giving the player some feedback will put a huge dent in the scenario we've seen over the past few years, where someone new to the game has this magic "aha" moment and tries to use double-click or an autoclicker opportunistically. It would be good for the server, in a roundabout way, to immediately tell them to fuck off. And if they go through the effort of figuring out the exact interval afterwards, they're at least still going to be firing at whatever rate the server operator has chosen. |
The player should, yes - this is a good point. Just to clarify my previous comment - when I said
I don't think this is really a valid concern - it's also already technically possible if a server simply edits the existing CPS limit for semi-auto weapons in a custom Dash build. Also, if a server host wants to give a certain subset of players with "secret info" an advantage in their server, a plethora of options already exist (and will always exist) for them to do that. It's not a specific scenario we need to accommodate for.
This is an interesting idea, and is the reason I dropped the PR into draft. I hadn't even considered this route, but thinking about it, it probably should be the case even now. Initially I was concerned that a client might get spammed because of it, but to be honest, I think every player out there would prefer a little chat message spam telling them that a shot they fired was dropped, as opposed to them simply not knowing. I have a strong suspicion as-is that the reported instances from the PUG players of fired fusions dealing no damage were caused by the weapon type-related bug that's fixed in this PR (as in, they were drumming with the precision rifle or pistol, swapped to the fusion and fired, and the fusion shot was dropped). In such a case, a chat message informing them that the shot was dropped would obviously not be as appreciated as the shot going through as it should have, but at least they wouldn't be completely unaware of what happened. I like the chat message approach when shots are dropped. Will work towards implementing that in this PR. |
|
Updated PR per the above. Players are now notified when shots are cancelled by the server for any reason. In the case of the CPS rate being exceeded, they are informed as to their current click rate and the server's cap. Screenshot showing this is below. |
|
Potential alternative:
It should have |
|
Good call on the My intent with keeping the message concise was to ensure it usually is only one line. Perhaps there is value in spelling out "clicks per second" rather than using CPS, but I'm not sure there's any real risk of a person not knowing what "CPS" is, especially given the context in which they'd receive this. |
|
Fair enough, initially I didn't like the loss in precision, but realistically it's just as useful to a player but easier to read. Changed.
I looked through the rest of the chat messages Dash sends to clients from server. Seems the standard practice is to end with a period where the message ends with words (usually) but not when it ends with numeric values. Following that convention, I've removed the period from the CPS message (but not the other shot cancelation messages).
Initially I thought so as well, but this message also is sent if an automatic weapon is somehow firing quicker than the allowed rate (as derived from Fire Wait in weapons.tbl). Granted, that's realistically only going to happen if a client is trying to cheat, but either way, because it's used for all fire rate-caused drops rather than just those for semi-auto weapons, I also swapped "CPS" for "fire rate" (to remain accurate and avoid the question entirely of whether a hypothetical brand new RF player receiving this message would know what CPS means), and made more clear that the maximum is from the server. |
|
Should a client not be sent a packet to fix their clip if a shot was canceled? |
I've not tried it, but the concept appears to suffer from the same problems I described earlier in explaining why I consider a clientside portion of this out of scope. I can't imagine a way to implement what you're asking for without causing very wonky behaviour, especially if you're clicking faster than your latency (which would typically be the case if your shot were to be cancelled). Unless there's an obvious approach without undesirable side effects which I am missing, my view on that idea is the same as my view on a clientside portion: worth evaluating later, not now, and certainly not grounds for delaying the huge benefits brought by this PR. |
A custom packet increasing either clip or ammo by 1. Edit: Why not use both |
|
Why have these changes been made? |
Apart from general cleanup:
This PR does not bring the click limiter functionality quite to the point where I'd like it to be in an ideal end state, but I think it's a considerable improvement vs current master. Once rafalh reviews, if he's not comfortable merging as-is, I would cut this PR back to just resolving the weapon type/fire mode related bugs and then I'd handle configuring/changing the click limiter functionality for semi-auto weapons later in a separate PR. |
Why? Are you talking about the server message? I ironically thought it was clearer with clicks per second. |
With the "felt" comment, I'm talking about how it felt to play with. The notification has to reflect the behaviour of the feature in my view, but I'm specifically talking about how the feature felt to play with during gameplay, not the text of the notification. |
| @@ -245,47 +246,65 @@ bool is_entity_out_of_ammo(rf::Entity *entity, int weapon_type, bool alt_fire) | |||
| auto clip_ammo = entity->ai.clip_ammo[weapon_type]; | |||
| return clip_ammo == 0; | |||
| } | |||
| void send_private_message_for_cancelled_shot(rf::Player* player, const std::string& reason) | |||
| { | |||
| auto message = std::format("\xA6 Shot canceled: {}", reason); | |||
There was a problem hiding this comment.
| auto message = std::format("\xA6 Shot canceled: {}", reason); | |
| const auto message = std::format("\xA6 Shot canceled: {}", reason); |
| int player_id = pp->net_data->player_id; | ||
| static int last_weapon_fire[rf::multi_max_player_id][num_samples]; | ||
| int now = rf::timer_get(1000); |
There was a problem hiding this comment.
| int now = rf::timer_get(1000); | |
| const int now = rf::timer_get(1000); |
| static std::vector<int> last_weapon_id(rf::multi_max_player_id, 0); | ||
| static std::vector<int> last_weapon_fire(rf::multi_max_player_id, 0); |
There was a problem hiding this comment.
| static std::vector<int> last_weapon_id(rf::multi_max_player_id, 0); | |
| static std::vector<int> last_weapon_fire(rf::multi_max_player_id, 0); | |
| static constinit std::array<int, rf::multi_max_player_id> last_weapon_id{}; | |
| static constinit std::array<int, rf::multi_max_player_id> last_weapon_fire{}; |
| } | ||
|
|
||
| // check if time since last shot is less than minimum wait time | ||
| int time_since_last_shot = now - last_weapon_fire[player_id]; |
There was a problem hiding this comment.
| int time_since_last_shot = now - last_weapon_fire[player_id]; | |
| const int time_since_last_shot = now - last_weapon_fire[player_id]; |
This PR adds a "$DF Semi Auto Click Limit" configurable option to dedicated_server.txt and documents its usage. This option allows server operators to set a max cps value to be used for semi auto weapons, and gives them an additional tool in combatting autoclicking cheating, to a threshold they are comfortable with, rather than forcing a specific threshold on all server operators. The default used if this option is not specified is 20, which is already the default in Dash for semi auto weapons.
A click limiter like this is something that has long been requested by server operators and players, including a mention in #256 ("pr rate limiter").
This PR also fixes the following bugs:
I didn't set out to find these bugs - I found and fixed them while I was implementing the feature. Testing this feature is how I found the second bug listed above, which led me to the others. I could break these fixes out into a PR separate from the feature if you like (let me know), but given that I fixed them while working on this, and that the fixes should (I think?) be uncontroversial, I thought it made sense to submit them together.