feat: add configurable StagedPhysicsPlugin

[JRMurr]

Closes: #170

Added the StagedPhysicsPlugin which allows you to pass in stage labels for the main physics schedule, the post physics stage, and tick stage.

The main physics needs to be a schedule and not just a stage since it uses sub stages. The other two are fine as just stages.

Creating as a draft since I'm not sure the best way to add tests for this. I made an example custom_schedule.rs which is basically just the demo example using the new plugin.

I'm still somewhat new to bevy/heron so let me know if the comments I added don't make sense

[jcornaz]

Hi, thanks for the contribution :-) Very much appreciated!

I would prefer to add new plugins only without touching the type definition of the old ones. In other words, I'd like to add the feature without breaking the existing API.

I think it should be possible. We can even share logic, if the old plugin can create and invoke the new one when it is installed.

[JRMurr]

That makes sense, so are you saying to make a new version of the rapier plugin and have the old one just call into it?

[jcornaz]

Yes. Conceptualy like this:

pub struct PhysicsPlugin; // Same definition as before
pub struct StagedPhysicsPlugin { /* fields */ } // New plugin

impl Plugin for PhysicyPlugin {
  // Old plugin implementation reworked to delegate to the new plugin
  fn build(&self, app: &mut App) {
    app.add_plugin(CorePlugin) // Add the core stages
    app.add_plugin(StagedPlugin { /* ... */ }); // Install the "StagedPlugin", using the stages installed by the `CorePlugin`
  }
}

impl Plugin for StagedPhysicsPlugin {
  fn build(&self, app: &mut App) {
    /*
    The new setup code, previously in `PhysicsPlugin`.
    Here in its modified version, that add the system on the parametrized stages
    (either defined manually by the end-user or by the `PhysicsPlugin`)
    */
  }
}

[JRMurr]

ooo your talking about "root" plugin here https://github.com/JRMurr/heron/blob/2605f414e7536d148f93a8489be5a708d1cb3c05/src/lib.rs#L159 and not the "interal" rapier plugin here https://github.com/JRMurr/heron/blob/2605f414e7536d148f93a8489be5a708d1cb3c05/rapier/src/lib.rs#L52.

Yea that should be a simple fix

[jcornaz]

mmh... sorry, I forgot about that one.

But, I'm talking about both. Actually about all of them (incl. the CorePlugin). The goal is that no existing API is changed. The implementation can change by delegating to new code, but not the existing API.

For instance the CorePlugin was public without any field. So adding a field in that struct would be a breaking change.

// A consumer may write this, and it compiles. So it should continue to compile.
let _ = heron_core::CorePlugin;

[JRMurr]

ahhh ok that makes more sense, will do!

[JRMurr]

It looks like the tests running cargo test --no-default-features --features debug-2d are getting a linker error for some reason. Its not happening for me locally (granted i do have a weird setup with nixos). I'll take a deeper look tomorrow

[jcornaz]

Thanks for the changes, it looks great. I'll have a deeper look soon (-ish).

Its not happening for me locally (granted i do have a weird setup with nixos). I'll take a deeper look tomorrow

It doesn't happen for me locally either. I am on archlinux + rustup, and I tried with with rust 1.57 to be sure using the same rust version as the github action.

Let's see... I'll try to look at it too.

[jcornaz]

the CI passed now... I don't know, maybe there is a flakyness. But I doubt it was due to your code.

When you think you don't have anything else to add, you can mark the PR as ready-for-review, and I'll try to have a deeper look and merge when I find the time (hopefuly during the week).

[JRMurr]

@jcornaz I think its good for review, my test here https://github.com/JRMurr/heron/blob/2283b5974385765cb61b4c0b65dd611c32dd5aa0/rapier/tests/resources.rs#L92 is a little cheeky its just checking that https://github.com/JRMurr/heron/blob/2283b5974385765cb61b4c0b65dd611c32dd5aa0/rapier/src/lib.rs#L141 are added.

[jcornaz]

If I understand well, your need is to choose in what stage the actual physics step happen (the systems that are currently running on CoreStage::PostUpdate by default), and then ofc you need to run the "schedule" before that stage.

But, do you think it would be feasible to always run the PhysicsTime::update in the CoreStage::First stage, and not let the user choose the stage for that system?

If we could make this (minor) simplification in the API, that would be nice I think.

On another topic: maybe the Plugins would be easier to understand/use if they weren't generic. The fields types could be Box<dyn StageLabel>. What do you think?

[JRMurr]

Yea for my current implementation i was feeling iffy on the PhysicsTime::update stuff. For rollback you would need a constant physics tick rate and re-simulating multiple steps as it is now might get weird with the time delta stuff.

I'll have to think on this some more. Might be a good idea to just have the user be able to kick off the physics tick manually when re-simulating

As for the Box<dyn StageLabel> that's a good idea, ill give that a shot

[jcornaz]

For rollback you would need a constant physics tick rate and re-simulating multiple steps as it is now might get weird with the time delta stuff.

It is possible to configure for constant physics rate, via PhyiscsSteps::from_steps_per_seconds for example. Maybe that's enough?

[jcornaz]

Hey, you know what?

While talking about our problem in the bevy discrod I got an idea...

What if, we only let the user choose in which stage to run the physics step (The one which currently runs in CoreStage::PostUpdate). And for all other stages, we leave them where they are.

That would means:

• cons: if the physics step run before the CoreStage::PostUpdate, there will be 1 frame delay when a physics entity is spawned or when mutating a component that requires to re-create the rapier entities (like changing the collision shape).

• pros: It is simple and ergnomic to use: Only 1 stage to understand and care about.

I am starting to think that the pros outweight the cons in this case. What do you think @JRMurr?

[JRMurr]

Hmm tbh im still a little confused on what each stage actually does. Do these comments describe it mostly correctly? https://github.com/JRMurr/heron/blob/2283b5974385765cb61b4c0b65dd611c32dd5aa0/rapier/src/lib.rs#L73

I think not configuring step_physics_stage is possible if i handle the timer resource correctly in a rollback but i may still need the physics_schedule since during a rollback (where multiple physics frames are re-simulated during a single "normal frame") entites could spawn for projectiles or w/e.

I understand your goals for making the heron api simple and easy to use so if you feel that this makes it more complex i understand

[jcornaz]

Hmm tbh im still a little confused on what each stage actually does. Do these comments describe it mostly correctly? https://github.com/JRMurr/heron/blob/2283b5974385765cb61b4c0b65dd611c32dd5aa0/rapier/src/lib.rs#L73

Here are what the stages do (in order):

1. in CoreStage::First we update the time in "step" resource that is responsible to deside if this frame should perform a simulation step.
2. between CoreStage::Update and CoreStage::PostUpdate we have a "schedule" that constains multiple sub-stages. The global responsiblity of that "schedule" stage is to udpate the rapier world.
3. in CoreStage::PostUpdate which is kind of the "ultimate" phase where we actually step the physics simulation and update all bevy components.

I think not configuring step_physics_stage is possible if i handle the timer resource correctly in a rollback but i may still need the physics_schedule since during a rollback (where multiple physics frames are re-simulated during a single "normal frame") entites could spawn for projectiles or w/e.

Ah ok. I didn't understand that correctly. So basically you do need to define where all of them run (physics-time update, rapier-update, bevy-update). Is that correct?

I understand your goals for making the heron api simple and easy to use so if you feel that this makes it more complex i understand

Well, I am trying to simplify as much as I can yes. But that doesn't mean I am against a slightly more complex solution. I am just trying to iterate on it, to see if we can improve it. But I think I now understand you do need to define where to run the three stages.

I guess if you can just use Box<dyn StageLabel> to avoid the generic plugin, I'll be able to merge.

Thanks for helping me to understand the context :)

[JRMurr]

Ah ok. I didn't understand that correctly. So basically you do need to define where all of them run (physics-time update, rapier-update, bevy-update). Is that correct?

Yea, the idea for rollbacks is you predict what the other players inputs were (usally just repeat what they were doing the last time you got inputs from them) and when you get their next input check if that predicition was correct. If not you re-load the game state and re simulate given that input.

So the idea would have a RollbackSchedule that would hold all gameplay related logic and be run on demand during rollbacks.

I wonder if we could add a validation step when heron uses the passed labels to make sure the relative ordering makes sense and panic?

Ill work on the Box<dyn StageLabel> this week hopefully.

Thanks again for making this lib, its wonderful to use!

[Shatur]

@JRMurr are you planning to continue working on it?

[Shatur]

I guess if you can just use Box to avoid the generic plugin, I'll be able to merge.

@jcornaz But why use Box<dyn StageLabel>? We usually know the stage at compile time. I saw generic plugins in other crates, in leafwing_input_manager, for example.

[jcornaz]

But why use Box<dyn StageLabel>? We usually know the stage at compile time. I saw generic plugins in other crates, in leafwing_input_manager, for example.

Because there are not one but three stages. And I feel like having to define three stages via positional generics is error-prone and not very ergonomic.

And the "cost" of having it resolved at run-time is completely negligible because it is only resolved at startup and they are ultimately boxed by bevy anyway.

EDIT: Not to mention that generic would makes impossible to change the stages without breaking the API

[Shatur]

Because there are not one but three stages. And I feel like having to define three stages via positional generics is error-prone and not very ergonomic.

Got it!

But do we need to configure CoreStage::First? We calculate when we should tick in networking Tick stage. And then user defines two stages, let's call them Frame and Simulation.
Frame is a stage which runs every frame and on rollback. I would put all Heron systems which normally runs on every frame here.
Simulation is a stage wich runs on networking tick and on rollback. I would put Heron systems which normally runs on physics tick here.
What do you think? This could reduce the number of stages to configure into two.

[jcornaz]

But do we need to configure CoreStage::First? We calculate when we should tick in networking Tick stage. And then user defines two stages, let's call them Frame and Simulation. Frame is a stage which runs every frame and on rollback. I would put all Heron systems which normally runs on every frame here. Simulation is a stage wich runs on networking tick and on rollback. I would put Heron systems which normally runs on physics tick here. What do you think? This could reduce the number of stages to configure into two.

Ideally, that's what I would like. I suggested something in this direction in one of my previous comment.

But I understood from @JRMurr, that they do need to configure the three stages.

[Shatur]

But I understood from @JRMurr, that they need do need to configure the three stages.

I'm curious why. Looking forward to his reply.

[cscorley]

I don't believe this approach will work for something like bevy_ggrs (yet). That library impl Stage for GGRSStage, which it uses to handle updating game state with ggrs, which it adds before CoreStage::Update. Unfortunately, add_system_set_to_stage only works for SystemStages. This means, specifically for bevy_ggrs at least, that bevy function will not find the GGRSStage and add the desired step systems.

That's not to say this PR doesn't work, just that it will not work with other plugins that implement custom stages (i.e., bevy_ggrs). (I am guessing bevy_backroll also falls into this category, but I have not read much, going by this impl Stage)

[Shatur]

Interesting. Not sure if we even can do anything for it until stageless.

[Shatur]

Actually, we can do better. Instead of setting user-defined labels we can make heron's stage labels public and add an option to not create actual stages, only put systems at labels. User will just create its own stages with this labels and user will be able to run them in its own exclusive system. No generics or boxes required, just a boolean option.