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!: SessionBuilder makes systems + world immutable during session build + Add a rollback-safe world reset utility #489

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

Conversation

MaxCWhitehead
Copy link
Collaborator

Goals

Allow the world to be reset safely for network rollback:

Systems + Resources and Rollback Safety

  • Systems (stage-based, single success, and startup) are immutable once session is created.
  • Reset-safe initialization of Resources: World is not available during session build, resources must be set on SessionBuilder. (They are captured so that on initial startup, or after reset, can be set to initial state).
  • Completion status of "startup" and single success systems is stored in a world resource. (SessionStarted, SingleSuccessSystems).
    • This makes them rollback safe, if rollback before single success completed, it will run again. If rollback before a reset, startup systems will run again.

Triggering World Reset

This may be done with the ResetWorld resource. SessionRunner is responsible for calling world.handle_world_reset. It is called right after stage exec in default/ggrs session runner, so these mutations to world are coupled to a ggrs frame.

Resetting Resources

During a reset, resources and components are all reset (including Entities resource). Because the initial resources are captured and saved during session build, after reset on next step during SystemStages startup, the initial resources will be re-inserted.

"Special" Resources

It turns out we have a lot of special resources, I handled a few of them to make sure they do the right thing (or what I think makes sense atm...)

  • Shared resources are not removed during reset (just FYI).
  • Sessions: Behind the scenes this is now inserted as a 'shared resource', and is not wiped out during reset.
  • Time: This is preserved. (It is assumed to exist/required by bones, and I think resetting this may have negative side effects).
  • SessionOptions: This is consumed by bones core loop and expected to exist, so preserved.
  • RngGenerator: GgrsSessionRunner is preserving this on reset, I think resetting to initial seed after a reset may make things feel less random, so opted to preserve.
  • SyncingInfo: This is re-inserted by ggrs runner before each step, this should not be impacted, no special care needed by reset.

Session Initialization Changes (SessionBuilder)

Changes were made to how sessions are built to enforce system immutability after creation, and restrict access to World while building session. This has some impact (breaking changes) to API, but I tried to make it not too painful.

Sessions may no longer be constructed directly. Don't want world resources to be modified in session plugin installs, as that resource change is then not captured in startup resources for a reset. There are a couple different ways to make a new session outlined below.

  1. create_with uses a closure to contain session init, this is nice as it ensures SessionBuilder is finished + added to Sessions:
    game.sessions.create_with("menu", |builder| {
        builder
            .install_plugin(DefaultSessionPlugin)
            .add_system_to_stage(Update, menu_system);
    });

or if just installing one plugin:

game.sessions.create_with("menu", menu_plugin_function);
  1. Use SessionBuilder directly:
   let mut menu_session_builder = SessionBuilder::new("menu");

   menu_session_builder
       .install_plugin(DefaultSessionPlugin)
       .add_system_to_stage(Update, menu_system);

   // Finalize session and register with `Sessions`.
   let finished_session_ref = menu_session_builder.finish_and_add(&mut game.sessions);

or

  let mut menu_session_builder = SessionBuilder::new("menu");

  menu_session_builder
      .install_plugin(DefaultSessionPlugin)
      .add_system_to_stage(Update, menu_system);

  // Finalize session and register with `Sessions`.
  // (`create` is the same as `finish_and_add`)
  let finished_session_ref = game.sessions.create(menu_session_builder);

Risk of forgetting to finish SessionBuilder

I don't love this API - by using SessionBuilder to restrict mutability of resources/systems + disabling ability directly construction a Session, we have a risk of configuring a SessionBuilder and forgetting to "finish" or add to Sessions and do anything useful with it. I added a guard (FinishGuard) to builder that if dropped (builder not consumed/finished), will print a warning.

The other option is changing the SessionBuilder functions to move builder and return it, instead of &mut SessionBuilder as it is now. This could be combined with #[must_use] to lint if it isn't consumed/finished. I had a hard time deciding which route to go - I decided against the move-semantics / linear approach as it means if you are not chaining all calls on builder, you have to rebind it with let again, IMO it is not a pleasant experience for more complicated stuff. I think the run-time warning on Drop hopefully is enough.

Syntax Change (how to fix compilation)

Session plugins now take &mut SessionBuilder instead of &mut Session:

    // old:
    fn install(self, session: &mut: Session);
    
    // new
    fn install(self, session: &mut SessionBuilder);

World is no longer on session builder, the functions used on world for resource init now are available on builder directly.

    // old:
    session.world.init_resource::<MyResource>();
    
    // new
    session.init_resource::<MyResource>();

@MaxCWhitehead
Copy link
Collaborator Author

MaxCWhitehead commented Nov 19, 2024

Other side notes:

Preserving a resource on reset:

There isn't a nice way to preserve (or partially preserve) a resource during reset. One option is to wrap any fields you want to not be impacted in an Arc. After reset + startup on next tick runs, the resource will be back to its state from session creation, but that arc will then preserve that field. Need to think about if there's a better way to support this...

The caveat being that these now do not get rolled back either... so probably not the answer.

(For something like a "score" value you don't want to be reset. Otherwise, can use shared resources, but that has its own complexity).

Timing of startup/re-init after reset:

Right now, a reset happens after all stages/etc are run, if triggered. But the "startup" operations (re-insert startup resources, run startup systems) doesn't happen until the beginning of next step. Maybe it should happen immediately after reset, so there isn't a gap in resources for reset world before next frame (other sessions may want to read from reset session?) This isn't hard to change, just not sure.

immediately startup after reset instead of waiting until next tick.
@MaxCWhitehead
Copy link
Collaborator Author

MaxCWhitehead commented Nov 22, 2024

Timing of startup/re-init after reset:
Right now, a reset happens after all stages/etc are run, if triggered. But the "startup" operations (re-insert startup resources, run startup systems) doesn't happen until the beginning of next step. Maybe it should happen immediately after reset, so there isn't a gap in resources for reset world before next frame (other sessions may want to read from reset session?) This isn't hard to change, just not sure.

Updated to immediatel handle system stages startup after reset. This logic for startup was moved into SystemStages::handle_startup out of step impl. This should not impact execution of the session in any way (moved something from beginning of next step to end of current/reset step), but may avoid some edge cases involving what resources other sessions may expect this one to have, and avoids a gap of no resources after a reset.

@MaxCWhitehead
Copy link
Collaborator Author

MaxCWhitehead commented Nov 23, 2024

Preserving a resource on reset:
There isn't a nice way to preserve (or partially preserve) a resource during reset. One option is to wrap any fields you want to not be impacted in an Arc. After reset + startup on next tick runs, the resource will be back to its state from session creation, but that arc will then preserve that field. Need to think about if there's a better way to support this...
...

ResetWorld now has a resource store + api to allow setting "reset resources", which will be applied as final word to world after a reset. This may be used to preserve an existing resource and override the initial state it would've been reset. You can also insert an "empty" resource, which will remove a resource after reset (if you have a startup resource from session build you don't want, can use this to override).

So something like a match score could be thrown in there to ensure the reset does not wipe that out, without having to make that a shared resource or use Arcs, keeping it simple + rollback compatible.

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.

1 participant