-
-
Notifications
You must be signed in to change notification settings - Fork 85
splash #2586
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
Open
Pospelove
wants to merge
6
commits into
skyrim-multiplayer:main
Choose a base branch
from
Pospelove:splash
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
splash #2586
+103
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to d02408f in 1 minute and 55 seconds. Click for details.
- Reviewed
151lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skymp5-server/cpp/server_guest_lib/MpActor.cpp:694
- Draft comment:
In GetLastHitTime, if no matching target is found the default time_point (epoch) is returned. Confirm that this default is intended, as it can result in a large duration. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is asking the PR author to "confirm" something, which violates the rule: "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended." The comment doesn't identify a definite bug - returning epoch as a sentinel value is a common pattern in C++ when dealing with time points. The author likely intended this behavior as a way to indicate "no hit found." The comment is speculative ("can result in a large duration") rather than pointing to actual problematic code. This type of comment should be deleted according to the rules. Could there be a legitimate concern here? If callers compute durations from this epoch time point, they would get very large durations. However, without seeing the calling code, I can't determine if this is actually a problem. The author may have intentionally used epoch as a sentinel value that callers check for. Even if there's a potential issue with how callers use this return value, the comment violates the explicit rule against asking authors to confirm intentions. The rules state I need "STRONG EVIDENCE that the comment is correct" to keep it. Without seeing caller code that mishandles the epoch return value, I don't have strong evidence of a bug. The comment is speculative and asks for confirmation rather than pointing to a definite issue. This comment should be deleted because it asks the PR author to confirm their intention rather than identifying a clear code issue. It's speculative about potential problems without demonstrating actual problematic usage. The rules explicitly prohibit comments that ask authors to verify, confirm, or ensure their intentions.
2. skymp5-server/cpp/server_guest_lib/MpActor.cpp:718
- Draft comment:
SetLastHitTime uses a linear search on a list capped at 16 entries. This is acceptable given the cap, but consider a comment or alternative if future requirements might increase the number of entries. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. skymp5-server/cpp/server_guest_lib/MpActor.cpp:1998
- Draft comment:
CountRecentHits iterates the entire hit list without cleaning up old entries. Given the maximum size is 16, this is likely acceptable, but consider cleanup for clarity if behavior changes. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
4. skymp5-server/cpp/server_guest_lib/ActionListener.cpp:1150
- Draft comment:
The splash attack detection logic—using timePassedAnyTarget and CountRecentHits—appears reasonable. Ensure that the chosen thresholds (kSplashTimeWindow=0.1s and kMaxSplashTargets=4) are tuned to avoid false positives. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
Workflow ID: wflow_y1B9MCiD5HZttUTg
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Introduces splash attack detection in
OnWeaponHit()to limit rapid attacks on multiple targets, updatingMpActorto track hit times and count recent hits.OnWeaponHit()inActionListener.cppto handle rapid attacks on multiple targets.kMaxSplashTargets(4) withinkSplashTimeWindow(0.1s).MpActorto track last hit times per target and overall inMpActor.cpp.CountRecentHits()to count hits within a time window.GetLastHitTime()andSetLastHitTime()inMpActorto handle target-specific hit times.CountRecentHits()toMpActorto support splash attack logic.This description was created by
for d02408f. You can customize this summary. It will automatically update as commits are pushed.