idanarye / bevy-yoetz

Apache License 2.0
7 stars 2 forks source link

Changed `Update` to `Fixed` for fixed time steps. #3

Closed seivan closed 3 months ago

seivan commented 3 months ago

Maybe the plugin should accept an argument to pick one, but it should still default to Fixed.

Recreating PR because previous got spam from some script kiddie.

idanarye commented 3 months ago

Why should Fixed be the default? Update is the to-go schedule for things that don't need special treatment, and I don't see why Yoetz should need special treatment. It's not timing-sensitive like physics-related things.

I do agree, tough, that ti's a good idea to let the user pick the schedule. Maybe it'd even be a good idea to make it mandatory - as in, have no default schedule. But even if you'll be able to convince me that Fixed should be the default, I'd still be against this change because it's a breaking change that does not trigger a build error. If I merge this, and someone upgrades Yoetz without changing their own code, they'll keep running their Suggest and Act systems in the Update schedule but the plugin will run in the Fixed schedule, causing a frame delay before a new strategy starts to get acted on.

idanarye commented 3 months ago

Recreating PR because previous got spam from some script kiddie.

I'm curious. What spam? I don't see anything on that PR. Did it get deleted?

seivan commented 3 months ago

Recreating PR because previous got spam from some script kiddie.

I'm curious. What spam? I don't see anything on that PR. Did it get deleted?

He got banned. Github had a CSS injection vurn recently.

seivan commented 3 months ago

Why should Fixed be the default? Update is the to-go schedule for things that don't need special treatment, and I don't see why Yoetz should need special treatment. It's not timing-sensitive like physics-related things.

Because you're tying the game logic (AI) with the render frame which is variable. This also makes it incompatible with multiplayer ticks. It makes it unpredictable and unreliable.

Generally you'd want the following in Fixed:

But even if you'll be able to convince me that Fixed should be the default, I'd still be against this change because it's a breaking change that does not trigger a build error. If I merge this, and someone upgrades Yoetz without changing their own code, they'll keep running their Suggest and Act systems in the

That's a shame because you'd be promoting a bad pattern. Also tying it to the frame rate makes it run unnecessarily many times.

Update schedule but the plugin will run in the Fixed schedule, causing a frame delay before a new strategy starts to get acted on.

That's not necessarily a bad thing. But I don't see that happening, fixed runs before. And you generally want to keep certain things inside update, so acting (assuming it mutates visuals) should be in update anyway:

idanarye commented 3 months ago

That's not necessarily a bad thing. But I don't see that happening, fixed runs before.

Unless the old behavior is unquestionably a bug, changing it without triggering a compilation error is always a bad thing. Sometimes it's necessary, but not in this case - since we can just remove the Default and make picking the schedule mandatory.

As for how it'll happen: assuming a game follows the docs and/or the examples in my examples dir, and puts these systems in the Update schedule in the YoetzSystemSet::Suggest and YoetzSystemSet::Act system sets. Yoetz currently updates the strategy in an internal YoetzInternalSystemSet that runs between these two, so any changes caused by the Suggest systems will take effect by the time the Act systems of the same frame run.

If we move Yoetz update to the Fixed schedule, it'll no longer run between these system sets of the Update schedule (where the user systems still are). This means that the Act systems will have to wait to the next frame before they can see the changes from the Suggest systems.

This is a change in behavior.

For games that already put their Suggest and Act systems in Fixed, there will still be a change in behavior - it'll just work the other way around - from one frame delay to no delay. Unless they don't use the YoetzSystemSet sets (which previously had no effect on the Fixed schedule) - in which case it'll become random. Which is worse.

idanarye commented 3 months ago

But this argument is pointless - I'm just going to merge and then change it add a mandatory schedule argument.