-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
[Feature & Refactor] Refactored w2g protocol & add additional sync point #390
Conversation
how will this behave if there are >3 people trying to sync joining at the same time tho... |
This comment was marked as duplicate.
This comment was marked as duplicate.
Please also look at this. Whether you like such approach or not. |
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 |
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.
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.
Okay will update it and merge it here soon then. |
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. |
Forgot to mention link to lobby being copied on its creation without extra step of clicking 'Invite to lobby' |
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.
god, it feels nice to have someone competent write code for once
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
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. |
/** | ||
* @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, | ||
) |
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.
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)) |
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.
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...
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? |
Actual changes listed below scheme. How i think it will work
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. |
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 I don't believe i will finish this till the end of the year. Since i sadly also have things to do. |
565e6af
to
3063d65
Compare
@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. |
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 |
I HATE JSDOC ITS SOOOOO LARGE!!!!
better to use .editorconfig but here we are
c3878ed
to
08b49b2
Compare
c684a2b
to
6a5347d
Compare
9f745cf
to
fc8bab6
Compare
Additions to p2p protocol
Added additional sync point when someone joined ongoing session.
Current algorythm in nutshell by examples
Closes #363