-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Refactor: Make event types control the lifecycle of capture and playback #36
Conversation
@alice-i-cecile any chance of giving this a review and possible merge in the near future? I'm interested in using this crate (and pushing a 0.14 upgrade), but seeing this PR linger for 5 months has me worried that (understandably, given your new role in Bevy) this is on the back burner, and I'm better off either forking or helping out with |
@JeanMertz @alice-i-cecile I'll try to push this further along ASAP as well and add the tests that are needed for it to be merge-ready. happy to handle review comments as I do so if you get a chance Alice. it might take me a couple days since I have a couple other things going on. I do also have some work done on the 0.14 upgrade as well ATM, just need to figure out the correct approach for the events-based tests now that the story around |
Just added some updates to merge |
@snendev I might have a use-case for this in the coming weeks. I could help push this over the finish line, unless you have any uncommitted work you're actively working on, or want to wrap this up soon? With your blessing, please let me know what's still pending for this work to be good to go, other than adding new tests. |
My apologies for the delay here! Thanks @JeanMertz for the ping. If I remember right, I realized at some point that Observers were probably more suited for this than Events anyway, and then I ended up down some deep rabbit hole instead of testing the new behavior. Then I went on a vacation and completely forgot about this. Really sorry about that! I'll try to put up the commits I have cooking tonight -- tbh it kinda implies another big refactor though. I will probably put those changes up on a separate branch for independent review that way we can compare the solutions more easily, unless I really think one is much stronger than the other. Once it's clear what direction is preferred I would definitely appreciate help adding any tests we think are useful. Sorry again for the delay! I'll put up the code ASAP. |
No worries @snendev , and it does appear that observers would be the way to go on first sight. |
@JeanMertz please compare with #39. Sorry for the delay, I got caught up in the |
--> #39! |
Issue
#34 - Refactor to allow users to trigger capture/playback as-needed
I've made this a draft PR for now since it needs tests for the new features. I thought it would be better to write those after submitting a working version so that the details can be ironed out first.
Changes
The strategy was to define new
Event
types that would be used to control the lifecycle of capture/playback operations. These are effectively[Begin|End]Input[Capture|Playback]
. TheBegin
types each require the data to build its own correspondingResource
s; theEnd
types carry no data.Plugins no longer attach
TimestampedInputs
or other resources related to capture/playback. Instead, these resources and attached and removed as capture and playback actions are detected.This also adds a mechanism for overwriting the window entity that inputs target, in case a specific window should be targeted on playback. This will be useful in editor or multi-window contexts and is generally nice to have, so it fits well in this set of changes.
I also deleted the random file I added in the last PR. My bad!
Questions
Resource
"should" be included in run conditions. Do you have strong opinions here? Does the system ordering seem correct?PlaybackFilePath
still needs to accept anOption
.TimestampedInputs
in the world after capture? A new-typeResource
forTimestampedInputs
could be appropriate.TODOs
Event
).