fix: fix 'magnetic furniture' bug #2340
Merged
Conversation
This file contains 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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to ce03a4e in 1 minute and 39 seconds
More details
- Looked at
27lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1568
- Draft comment:
Consider verifying that 'loader' is defined before use, and also check that base.rec is not nullptr before calling GetType(). This prevents potential crashes if LookupById returns a null record. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1568
- Draft comment:
The new FURN-specific condition (returning false) is a logical change—double-check that this behavior is intended when the object’s type is 'FURN' and the activator is an actor. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1605
- Draft comment:
Log messages refer to 'ProcessActivate' even though they are in CheckIfObjectCanStartOccupyThis. Consider using the correct function name for clarity. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
Workflow ID: wflow_oeKDXWAq3nbWzSmh
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 23ad5c3 in 1 minute and 16 seconds
More details
- Looked at
47lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1139
- Draft comment:
Enabling std::terminate() can be drastic. Confirm that aborting is desired in production. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%
The comment is asking the PR author to confirm their intention regarding the use of std::terminate(). This violates the rule against asking for confirmation of intention. However, it does highlight a potentially significant issue with using std::terminate() in production, which could be useful for the author to consider. The comment could be rephrased to suggest considering the implications of using std::terminate() without directly asking for confirmation.
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1605
- Draft comment:
Log message formatting changed—ensure consistency and clarity remain. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1784
- Draft comment:
JSON initializer formatting modified; verify that the message format and concatenation are as intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1141
- Draft comment:
Ensure std::terminate() is the intended error handling here; abrupt termination may be too harsh. Consider if throwing an exception or a graceful shutdown is more appropriate. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1608
- Draft comment:
Verify that the revised spdlog message formatting meets the project’s style guidelines for consistent log output. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1788
- Draft comment:
The JSON construction for the inventory update has been reformatted. Confirm that the functional behavior remains unchanged. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_krVIg1UuwePXS4F2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Skipped PR review on deea7dc because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a45548a in 38 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1787
- Draft comment:
Cosmetic JSON formatting adjustment in SendInventoryUpdate; no functional change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1788
- Draft comment:
Good reformatting of the JSON initialization. Just ensure that concatenating Networking::MinPacketId with the JSON dump produces the intended message format. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_tC2h98oF67cVidGP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Pospelove
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f8d75d8 in 1 minute and 2 seconds
More details
- Looked at
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. skymp5-client/src/services/services/remoteServer.ts:189
- Draft comment:
Consider renaming 'onOpenContainerMesage' to 'onOpenContainerMessage' for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. skymp5-client/src/services/services/remoteServer.ts:217
- Draft comment:
Ensure that delaySeconds default (-1.0) is never used unintentionally; it's safe here, but adding a comment can prevent misuse in future modifications. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. skymp5-client/src/services/services/remoteServer.ts:187
- Draft comment:
Initializing delaySeconds to -1.0 may be confusing. Since it's always set in the if/else, consider initializing it to a neutral positive value or adding a comment explaining the negative default. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. skymp5-client/src/services/services/remoteServer.ts:223
- Draft comment:
The log message label 'onOpenContainerMesage' contains a typo. Consider correcting it to 'onOpenContainerMessage' for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_zbb9UKxwtV5RxYpG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e0958af in 51 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. skymp5-client/src/services/services/remoteServer.ts:224
- Draft comment:
Consider using a template literal for the log message to ensure consistent spacing. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. skymp5-client/src/services/services/remoteServer.ts:224
- Draft comment:
Minor logging formatting adjustment. Note that 'onOpenContainerMesage' remains misspelled; consider renaming it to 'onOpenContainerMessage' for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_NzDoE0erXjgEPKb3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c9a7af0 in 44 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1838
- Draft comment:
Reformatted JSON initialization improves clarity; no logic change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1838
- Draft comment:
The JSON initialization is reformatted for readability without changing logic. Please clarify in a comment how this change fixes the 'magnetic furniture' bug. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for clarification on how a specific change fixes a bug. This is asking the PR author to explain their change, which violates the rule against asking for explanations or confirmations of intent. Therefore, this comment should not be approved.
Workflow ID: wflow_9nbNOAGNYYFLXyys
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 9676234 in 2 minutes and 30 seconds
More details
- Looked at
232lines of code in4files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpActor.cpp:374
- Draft comment:
Good addition: calling BeforeDisable on all disable event sinks before disabling the actor helps resolve the magnetic furniture issue by ensuring proper cleanup of occupant references. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1500
- Draft comment:
Ensure that removal of both occupant destroy and disable sinks is handled consistently. The new disable logic seems to mirror the destroy sink, which is good for avoiding lingering occupant pointers (addressing magnetic furniture). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1570
- Draft comment:
In ProcessActivateSecond for FURN type, only the destroy sink is removed. Consider also removing occupantDisableSink before resetting the occupant to ensure full cleanup and avoid stale event sinks that may cause 'magnetic furniture' behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:113
- Draft comment:
Good addition of OccupantDisableEventSink. Ensure that in every state transition (activation, disable, destroy) both event sinks (destroy and disable) are removed to prevent lingering callbacks or memory leaks. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1350
- Draft comment:
ProcessActivateNormal is very complex with multiple type‐specific branches. Consider refactoring it into smaller helper functions (e.g. separate handlers for CONT, FURN, ACTI) to improve readability and maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1177
- Draft comment:
In ApplyChangeForm, calling std::terminate() when setProperty has been invoked is rather drastic. Consider a more graceful error handling or clearer messaging to avoid unexpected termination in production. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1762
- Draft comment:
The hardcoded filtering of non-SweetPie scripts in InitScripts may not be robust. Consider making such script filtering configurable to better accommodate different server setups. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_bpjbHWXSWVtydad4
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -82,7 +84,17 @@ class MpActor : public MpObjectReference | |||
| }; | |||
|
|
|||
| void AddEventSink(std::shared_ptr<DestroyEventSink> sink); | |||
There was a problem hiding this comment.
The overloaded AddEventSink/RemoveEventSink for DestroyEventSink and DisableEventSink are ambiguous. Consider renaming them (e.g. AddDestroyEventSink/AddDisableEventSink) for clarity.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4de75e9 in 35 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpObjectReference.h:42
- Draft comment:
Verify that the new forward declaration for OccupantDisableEventSink is necessary and ensure its corresponding header is included where needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.h:41
- Draft comment:
Added forward declaration for OccupantDisableEventSink for friend usage. Ensure proper header inclusion to avoid circular deps. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
The comment is informative and suggests ensuring proper header inclusion to avoid circular dependencies. It doesn't provide a specific code suggestion or ask for a specific test to be written. It also doesn't confirm if a change is intentional or suggest a refactor. Therefore, it seems to violate the rules.
Workflow ID: wflow_KRQEAOPYsALwhylN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0ea8782 in 58 seconds
More details
- Looked at
27lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpActor.cpp:507
- Draft comment:
Renamed 'RemoveAllEventSink' to 'RemoveEventSink' for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. skymp5-server/cpp/server_guest_lib/MpActor.h:84
- Draft comment:
Renamed 'RemoveAllEventSink' to 'RemoveEventSink' for DestroyEventSink to match implementation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, stating that a rename was done to match the implementation. It doesn't provide any actionable feedback or suggestions for improvement. According to the rules, purely informative comments should be removed.
3. skymp5-server/cpp/server_guest_lib/MpActor.cpp:510
- Draft comment:
Renamed RemoveAllDestroyEventSinks to RemoveEventSink to reflect that only a single sink is removed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, explaining a change in the name of a function. It does not provide any actionable feedback or suggestions for improvement. According to the rules, purely informative comments should be removed.
4. skymp5-server/cpp/server_guest_lib/MpActor.h:85
- Draft comment:
Updated declaration: Rename RemoveAllEventSink to RemoveEventSink for consistency with implementation. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_oLJ74WEQw10S5jj6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 07f9b88 in 2 minutes and 5 seconds
More details
- Looked at
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpActor.cpp:518
- Draft comment:
The event sink type is changed from DestroyEventSink to DisableEventSink. Consider renaming the method (e.g. AddDisableEventSink) for clarity, as the name no longer reflects the type. - 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 suggestion to rename the method seems reasonable at first glance since it would make the method name more specific to its parameter type. However, looking more carefully:
- There are two overloaded AddEventSink methods, each taking a different event sink type
- The overloading pattern is consistent and follows standard C++ practices
- The parameter types clearly indicate which sink is being added
- Renaming just one overload would make the API less consistent
The comment may be missing that this is an intentional overloading pattern where the parameter type provides sufficient clarity. Having differently named methods for each sink type could actually make the API more confusing.
The current overloaded method names follow good C++ practices where the parameter types differentiate the methods. Renaming would break this pattern and could make the API less consistent.
The comment should be deleted as it suggests breaking a valid method overloading pattern that already provides sufficient clarity through parameter types.
2. skymp5-server/cpp/server_guest_lib/MpActor.cpp:523
- Draft comment:
Similarly, the removal method is updated to use DisableEventSink. For consistency and clarity, consider renaming it (e.g. RemoveDisableEventSink). - Reason this comment was not posted:
Marked as duplicate.
3. skymp5-server/cpp/server_guest_lib/MpActor.cpp:517
- Draft comment:
Changed AddEventSink to take DisableEventSink instead of DestroyEventSink. Verify that all call sites now create/expect DisableEventSink. - 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 all call sites now create/expect DisableEventSink, which is against the rules as it asks for confirmation of intention and verification. It does not provide a specific suggestion or point out a specific issue.
4. skymp5-server/cpp/server_guest_lib/MpActor.cpp:523
- Draft comment:
Modified RemoveEventSink to operate on DisableEventSink. Ensure consistent usage across the codebase. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_KsQL6016Z0OBsA3n
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
I carefully tested. Please Revert if neeed |
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
Fixes 'magnetic furniture' bug by adding delay and disable event handling in
remoteServer.ts,MpActor.cpp, andMpObjectReference.cpp.delaySecondsinremoteServer.tsfor handling furniture activation with a delay of 1.0 seconds.DisableEventSinktoMpActorandMpObjectReferenceto handle disable events before disabling an actor or object.MpActorandMpObjectReferencenow haveAddEventSinkandRemoveEventSinkforDisableEventSink.MpActor::Disable()andMpObjectReference::Disable()now triggerBeforeDisable()on all registeredDisableEventSinkinstances.remoteServer.tsto trace activation message sending.This description was created by
for 07f9b88. It will automatically update as commits are pushed.