-
-
Notifications
You must be signed in to change notification settings - Fork 85
refactor: optimize grid movement and subscription logic in MpObjectReference #2508
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
12
commits into
skyrim-multiplayer:main
Choose a base branch
from
Pospelove:noset
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.
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 7b30bf0 in 1 minute and 33 seconds. Click for details.
- Reviewed
243lines of code in3files - Skipped
0files when reviewing. - Skipped posting
6draft 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/Grid.h:37
- Draft comment:
New function MoveWithDiff correctly computes neighbor differences using std::set_difference. Please ensure that the underlying std::set uses a consistent strict weak ordering so that the difference is computed correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that the underlying std::set uses a consistent strict weak ordering. This falls under the category of asking the author to ensure something is correct, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue with the code.
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:691
- Draft comment:
ForceSubscriptionsUpdate now uses the diff from MoveOnGrid to drive subscribe/unsubscribe. Verify that this new subscription logic correctly handles edge cases (e.g. when the object was not active before). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that the new subscription logic handles edge cases. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
3. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1087
- Draft comment:
GetListeners (and similarly GetEmitters) now derive the listener set using WorldState->GetNeighborsByPosition. Confirm that this design change (moving away from locally stored pointers) is intentional and that it properly reflects subscription state. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:986
- Draft comment:
In Subscribe, the check to avoid duplicate actor listener entries via std::find is a good improvement. Double–check that removal in Unsubscribe consistently removes from the same container to avoid stale entries. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to double-check the removal process in the Unsubscribe function, which violates the rule against asking the author to double-check things. It does not provide a specific suggestion or point out a specific issue that needs addressing.
5. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1833
- Draft comment:
MpObjectReference::MoveOnGrid now returns a GridDiff. Ensure that all callers of MoveOnGrid (and any implicit assumptions on its side effects) are updated and working correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that all callers of a function and any implicit assumptions are updated and working correctly. This falls under the rule of not asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
6. skymp5-server/cpp/server_guest_lib/MpObjectReference.h:226
- Draft comment:
The method signature for MoveOnGrid has been updated to return a GridDiff. Make sure that all interface contracts are documented so future maintainers understand the rationale for returning diff information instead of a void result. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to document interface contracts, which is similar to asking them to ensure something is documented. This violates the rule against asking the author to ensure behavior is intended or documented.
Workflow ID: wflow_uyZA1bWOIl6fahqa
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
Optimized grid movement and subscription logic in
MpObjectReferenceusingGridDifffor efficient neighbor tracking and subscription updates.GridDiffinGrid.hto track added and removed neighbors during movement.MoveWithDiff()inGridImplto returnGridDiffwhen moving objects.MpObjectReference::MoveOnGrid()now usesMoveWithDiff()to get neighbor changes.ForceSubscriptionsUpdate()inMpObjectReference.cppprocessesGridDiffto update subscriptions.InitListenersAndEmitters()fromMpObjectReference.cpp.Subscribe()andUnsubscribe()inMpObjectReference.cppby removing redundant set operations.This description was created by
for 7b30bf0. You can customize this summary. It will automatically update as commits are pushed.