Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix ped sync behavior #3965

Closed
wants to merge 7 commits into from
Closed

Fix ped sync behavior #3965

wants to merge 7 commits into from

Conversation

Nico8340
Copy link
Contributor

Fixes unexpected ped sync behavior after #3936

@Nico8340
Copy link
Contributor Author

cc @tederis

@tederis tederis added bugfix Solution to a bug of any kind backwards-incompatible Should be merged after the release of 1.6.1 labels Jan 18, 2025
@tederis
Copy link
Member

tederis commented Jan 18, 2025 •
edited
Loading

This is potentially backwards incompatible. But due to the bit flags overlapping it may be hard to distinguish whether it pre-PR or post-PR behaviour. I think this could be merged without noticable issues.

See the explanation in Discord: https://discord.com/channels/801330706252038164/801411628600000522/1330055415760818208

@Nico8340 Nico8340 requested a review from tederis January 18, 2025 20:48
@Nico8340
Copy link
Contributor Author

Could we get more maintainers' opinions on this?

@botder
Copy link
Member

botder commented Jan 26, 2025

We could also just ignore the issue and live with the overlap and simply get rid of the BitStream-versioned blocks with the next version bump

@Nico8340
Copy link
Contributor Author

We could also just ignore the issue and live with the overlap and simply get rid of the BitStream-versioned blocks with the next version bump

Okay, let's close this then?

@botder
Copy link
Member

botder commented Jan 26, 2025

Fine by me.

@Nico8340 Nico8340 closed this Jan 26, 2025
@Nico8340 Nico8340 deleted the pedsync branch January 26, 2025 16:51
@tederis
Copy link
Member

tederis commented Jan 27, 2025

The problem with it is that without this PR the ped syncronization becomes unstable which potentially can (but depends on internal BitStream implementation) lead to crashes.

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

@tederis tederis Awaiting requested review from tederis

Assignees
No one assigned
Labels
backwards-incompatible Should be merged after the release of 1.6.1 bugfix Solution to a bug of any kind
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Nico8340 @tederis @botder