Skip to content

Reapply #2095 and fix reconnection error cause by it#2134

Open
NetsuNegi wants to merge 3 commits intoPhobos-developers:developfrom
NetsuNegi:branch/reapply-2095
Open

Reapply #2095 and fix reconnection error cause by it#2134
NetsuNegi wants to merge 3 commits intoPhobos-developers:developfrom
NetsuNegi:branch/reapply-2095

Conversation

@NetsuNegi
Copy link
Contributor

No description provided.

@NetsuNegi NetsuNegi added Needs testing ⚙️T1 T1 maintainer review is sufficient Bugfix This is a bugfix that does not need documentation beyond mention in changelog Needs MP testing This PR needs to be tested for desync. No Documentation Needed No documentation needed whatsoever and removed Bugfix This is a bugfix that does not need documentation beyond mention in changelog labels Mar 12, 2026
@phoboscn-bot
Copy link

To Chinese users:
This pull request has been mentioned on Phobos CN. There might be relevant details there:

致中文用户:
此拉取请求已在 Phobos CN 上被提及。那里可能有相关详细信息:

https://www.phoboscn.top/t/topic/238/1

@github-actions
Copy link

github-actions bot commented Mar 13, 2026

Nightly build for this pull request:

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

@Coronia
Copy link
Contributor

Coronia commented Mar 13, 2026

didn't desync in our test so far, and the previous issues are solved. More testing is appreciated tho

@Coronia Coronia added Tested and removed Needs testing Needs MP testing This PR needs to be tested for desync. labels Mar 13, 2026
@Coronia Coronia requested a review from TaranDahl March 14, 2026 02:36
@TaranDahl
Copy link
Contributor

I think we should find out exactly what processing was missing before that caused the desync.
This should be documented in the comments to remind other developers.

Copy link
Contributor

@TaranDahl TaranDahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code lgtm. But the issue above should be addressed.

@Coronia
Copy link
Contributor

Coronia commented Mar 15, 2026

I think we should find out exactly what processing was missing before that caused the desync. This should be documented in the comments to remind other developers.

We can't even figure out what exactly caused the desync yet, but just take reference of other event codes and add missing checks as much as possible (e.g. planning node, tether etc). So sadly there's no clear document as of now

@Coronia Coronia requested a review from secsome March 15, 2026 03:31
@NetsuNegi
Copy link
Contributor Author

NetsuNegi commented Mar 15, 2026

I think we should find out exactly what processing was missing before that caused the desync. This should be documented in the comments to remind other developers.

The most important code to resolve this desync seems like pSource->SetTarget(nullptr).
I don't known why, but when I removed it, tester said that desync happend again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No Documentation Needed No documentation needed whatsoever ⚙️T1 T1 maintainer review is sufficient Tested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants