Add ChangeBatcher to plugin for two-way sync #478
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
This prevents __flush from erroring out when an instance's Parent is changed to nil and it has other property changes in the same batch.
kennethloeffler
changed the title
#468 made me realize how bad of an idea this is in general...
This somewhat improves the situation (of course, it would preferrable to not have to hack around this problem with Source at all). It also sets us up nicely if we come across any other properties that do anything similar.
|
It's more or less where I want it for now - any further revisions are welcome |
I probably should have done this in the first place... ChangeBatcher still needs to unpause instances, but we don't need to hold pauses for any longer than one cycle.
|
For some reason I had it in my head that |
LPGhatguy
requested changes
Oct 15, 2021
LPGhatguy
approved these changes
Oct 18, 2021
Dekkonot
pushed a commit
to UpliftGames/rojo
that referenced
this pull request
Jan 11, 2024
* Implement ChangeBatcher * Use ChangeBatcher for two-way sync * Pause updates during patch application * I can English good * Break after encountering a nil Parent change This prevents __flush from erroring out when an instance's Parent is changed to nil and it has other property changes in the same batch. * Update rbx_dom_lua * Don't connect changed listeners in a running game rojo-rbx#468 made me realize how bad of an idea this is in general... * Update TestEZ and fix sibling Ref reification test * Add ChangeBatcher tests * Test instance unpausing by breaking functionality out to __cycle * Break up the module a bit and improve tests * Shuffle requires around and edit comment * Break out more stuff, rename createChangePatch -> createPatchSet * Make ChangeBatcher responsible for unpausing all paused instances This somewhat improves the situation (of course, it would preferrable to not have to hack around this problem with Source at all). It also sets us up nicely if we come across any other properties that do anything similar. * Remove old reference to pausedBatchInstances * Use RenderStepped instead of Heartbeat and trash multi-frame pauses I probably should have done this in the first place... ChangeBatcher still needs to unpause instances, but we don't need to hold pauses for any longer than one cycle. * Remove useless branch * if not next(x) -> if next(x) == nil * Add InstanceMap:unpauseAllInstances, use it in ChangeBatcher * Move IsRunning check to InstanceMap:__maybeFireInstanceChanged
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.
Closes #294, #368, and #433.
Depends on changes from rojo-rbx/rbx-dom#210.
This PR implements
ChangeBatcher. Its job is to collect any instance changes and periodically try to turn them into a patch to send to the Rojo server. Right now, this happens every 200ms.There is one small bump I hit during implementation. When a change is made to the
Sourceproperty of script, AND the change is made from a script or the command bar, AND the script is open in Studio, theChangedevent fires three times forSource. The first event occurs right when the change is made (or, if deferred events are enabled, at the next invocation point), while the latter two events occur some time after the currentHeartbeatcycle.If this is left unchecked, the
ChangeBatcherdetects a spurious change toSourceafter a patch application. To fix it, I addedInstanceMap:pauseInstanceForBatch. It pauses an instance until theChangeBatcherdecides to unpause it (in this case, after the nextHeartbeatcycle). This isn't terribly consequential - it just causes the client to send an unnecessary write request. I'm not 100% on whether we should keep this mechanism or not.I also made
InstanceMap:pauseInstanceno longer take a callback and addedInstanceMap:unpauseInstanceto avoid having to create a closure for every set of property updates during patch application.