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.

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

[Feature & Refactor] Refactored w2g protocol & add additional sync point #390

Draft
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

EnergoStalin
Copy link

@EnergoStalin EnergoStalin commented Dec 11, 2023

Additions to p2p protocol

Added additional sync point when someone joined ongoing session.

Current algorythm in nutshell by examples

  1. Client 1 creates lobby and becomes host automatically
  2. Client 2 join lobby with isHost false because code is present in joinLobby call receiving current torrent and shared session state from client 1 on 'peerconnect'
  3. Client 3 join lobby the same way as client 2 receiving state from client 1
  4. Client 1 leaves no host in the room so no new clients will receive initial state
  5. Client 4 joins no state synced to him
  6. Client 2 pick new torrent becoming host
  7. Now client 2 sends state to all new joined peers until someone else picked torrent acquiring host

Closes #363

@ThaUnknown
Copy link
Owner

how will this behave if there are >3 people trying to sync joining at the same time tho...

@EnergoStalin

This comment was marked as duplicate.

@EnergoStalin
Copy link
Author

Please also look at this. Whether you like such approach or not.

@ThaUnknown
Copy link
Owner

this is good code, but it tries to fix a broken system, to put it simply: "Miru's code fucking sucks", and you're trying to build on top of what's fucked, the 2nd PR you linked fixes a LOT of that, but I feel like you take OOP way too far, it's not that required in JS

to my knowledge webrtc guarantees packet order, so I don't think there's much need in creating a "batch" event, as you can simply send the events one by one and it will work the same way

honestly feel free to dump all my code, and write it from scratch like you did in that draft PR, just a little bit less OOP, you dont need a separate class for each event XD

@EnergoStalin
Copy link
Author

I feel like you take OOP way too far

Feel the same way that's why it's just draft and i asked someone to look.

Thought a little bit and class approach with events the most declarative way to do it so i against changes here.

to my knowledge webrtc guarantees packet order

Also thought that way when see msgId option in send interface but it was easier to implement it that way instead of assuming. Firstly i added array support on top level making all packets capable of batching but breaks backward compatibility too much. Don't think it's an issue with automatic updates tho 😉 . Will do it like you said then.

honestly feel free to dump all my code

Okay will update it and merge it here soon then.

@EnergoStalin
Copy link
Author

Decided to leave excessive OOP approach as is. Thinked about introducing LeaveEvent because during testing with 2 clients 'peerclose' not always called even withing a ~minute leaving 5 different peer instances in the list. Now it can be considered more safe(i think maybe it's my setup's fault noticed faulty internet today so removed it for now). Also i learned that awaiting p2pt.send is a bad idea.

I want to test with 3 clients before merging. Tomorrow will look at it.

@EnergoStalin
Copy link
Author

EnergoStalin commented Dec 14, 2023

Forgot to mention link to lobby being copied on its creation without extra step of clicking 'Invite to lobby'

@EnergoStalin
Copy link
Author

EnergoStalin commented Dec 14, 2023

yep 'peerclose not firing for me it also somehow bugged like that. To reproduce quickly rejoin lobby multiple times.
image

Same issue on master branch withouth backfiring wrong user on init tho.

Copy link
Owner

@ThaUnknown ThaUnknown left a comment

Choose a reason for hiding this comment

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

god, it feels nice to have someone competent write code for once

@ThaUnknown
Copy link
Owner

yep 'peerclose not firing for me it also somehow bugged like that. To reproduce quickly rejoin lobby multiple times. image

Same issue on master branch withouth backfiring wrong user on init tho.

this is likely just local webrtc woes, afaik this works fine on remote connections, timeout is like 30 seconds tho

@EnergoStalin

This comment was marked as outdated.

@EnergoStalin EnergoStalin changed the title [Feature] Sync up with freshly joined peers in ongoing session [Feature & Refactor] Refactored w2g protocol & add additional sync point Dec 14, 2023
@EnergoStalin

This comment was marked as resolved.

@EnergoStalin
Copy link
Author

EnergoStalin commented Dec 14, 2023

Need to do something with video buffering because if someone joins to lobby now not having torrent downloaded it ignore initial seeking and reset all lobby to the beginning when buffering ends.

Comment on lines 20 to 26
/**
* @type {BidirectionalFilteredEventBus<
* import('@/modules/w2g/events.js').EventData<import('@/modules/w2g/events.js').PlayerStateEvent>,
* import('@/modules/w2g/events.js').EventData<import('@/modules/w2g/events.js').PlayerStateEvent>
* >}
*/
const bus = new BidirectionalFilteredEventBus(
(state) => w2gEmitter.emit('playerupdate', state),
(detail) => session.localPlayerStateChanged(detail),
undefined,
// Dont send time 0 when non host
() => !session.isHost && !bus.isFirstOutFired,
)
Copy link
Author

Choose a reason for hiding this comment

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

Looks kinda scary but working pretty well as far as i tested. What do you think?

It serves 2 purposes do not backfire zero time at joining session and filter out repeating events among 2 event pipes.

playerState.index = detail.index
}
})
w2gEmitter.on('player', ({ detail }) => bus.out(detail))
Copy link
Author

@EnergoStalin EnergoStalin Dec 14, 2023

Choose a reason for hiding this comment

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

I feel like to do a proper sync direct access to player stats needed. Syncing it through periodical events too hard to maintain without some state. And i guess i'm out of options how to fix it. So player changes are needed.

Also currently it will sync old state to new connected peers if no new state synced during playback. Not to mention buffering problem aaaaagh. Guess it's a lot more complex to do it proper way than i thinked.

I need to reconsider my approach...

@EnergoStalin EnergoStalin marked this pull request as draft December 14, 2023 22:40
@EnergoStalin
Copy link
Author

Very simplified schema for counting in bufferization
Untitled-1

Common rules

  • Do not update session's player state while buffering
  • Wait for initial local buffering to complete for seeking to player state time if not already
  • Pause and block resuming player on all clients while any peers buffering

@ThaUnknown
Copy link
Owner

ThaUnknown commented Dec 15, 2023

I won't have the time to look at this for the next week, so this is gonna hang for a while, but the stats update schema seems.... overcomplex?

@EnergoStalin
Copy link
Author

Actual changes listed below scheme.

How i think it will work

  • peer joined seesion all other peer paused
  • peer buffered sending buffered event
  • when all peers buffered playback resumes

And now i realized that we only need to have info about peer buffering to force waiting before continue. So only additional buffering event needed.

@EnergoStalin
Copy link
Author

EnergoStalin commented Dec 20, 2023

I know! The most funniest solution is reply to unpause events from buffering client by pause events also sending toasts to all that it's buffering. That way we don't need store extra state among clients. Local buffering still need to be fixed tho.

This will involve excessive traffic but should work excellent since if peer suddenly leaves, no one stuck for 30 seconds until peerclose fired. Pinging that peer also not good idea tho cause all clients need to ping it simultationously. I found old existing WebRTC solution(which don't want to build locally). Will look at it's implementation and think of it. Before rushing to code.

I don't believe i will finish this till the end of the year. Since i sadly also have things to do.

@EnergoStalin
Copy link
Author

EnergoStalin commented Dec 29, 2023

@ThaUnknown how strong are you against typescript? I can't see any of you repositories using it which is quite surprising for me at least. I'm asking because it can reduce a lot of code in this PR removing large inconvenient jsdoc descriptions and i managed partially enable it without harming anything. I do my best not using anything typescript specific except type definitions in this PR.

Check it out here it has squash merged feat/qol branch for now can be easily removed with interactive rebase.

@ThaUnknown
Copy link
Owner

it has it's place, not in Miru tho, it's tooling fucking pisses me off, and jsdoc is enough 99% of the time

you're too used to typescript and use a fuckload of OOP, which is why you need so much JSDoc, I usually let it interpret shit with using object literals, which don't have its type but typedoc inherits it's structure

you write good code, don't get me wrong, it's just not my style, I do waaaaaaaaaaay too much insanely experimental and rapid prototyping, and typescript often gets in the way of that slowing me down, and reduces my time to work, which I already have little of, which is why I don't use it, in work or major projects I'd use it tho

I'm considering sticking my fingers into this PR and correcting some stuff more to how I'd write it, but for now I'm more concerned by the "bidirectional bus" shit which has got me confused as to it's high complexity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: w2g sync with joined peers
2 participants