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

Refactor: Make event types control the lifecycle of capture and playback #36

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

snendev
Copy link
Contributor

@snendev snendev commented Mar 1, 2024

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]. The Begin types each require the data to build its own corresponding Resources; the End 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

  • Many of the systems have run conditions attached, but it is somewhat arbitrary from a development perspective which Resource "should" be included in run conditions. Do you have strong opinions here? Does the system ordering seem correct?
  • Since resource existence now triggers a lot of the behavior, it's not clear whether PlaybackFilePath still needs to accept an Option.
  • What should happen if users want to keep TimestampedInputs in the world after capture? A new-type Resource for TimestampedInputs could be appropriate.

TODOs

  • Need to write tests for the new lifecycles and various Resources.
  • Need to read deserialization errors and forward them to clients (probably another Event).

@JeanMertz
Copy link

@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 bevy-autoplay (which is less expansive as this crate currently, so my preference would be for this crate to get some TLC).

@snendev
Copy link
Contributor Author

snendev commented Jul 16, 2024

@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 EventUpdateSignal resource in bevy works differently. (I had to fix some problems there when upgrading to 0.12.) I'll get back to that ASAP so it can be ready soon.

@snendev
Copy link
Contributor Author

snendev commented Aug 22, 2024

Just added some updates to merge main and update some tests. Not quite done here yet -- still need to add the new tests. But at least current tests pass. Will do my best to complete this soon!

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.

2 participants