Add runtime system insertion by Alainx277 · Pull Request #2507 · bevyengine/bevy

Alainx277 · 2021-07-20T16:57:04Z

Objective

Allow insertions of systems at runtime
See [Feature] Runtime Dynamic Components and Systems ( To Support Scripting Initiatives ) #142

Solution

Adds a new type of command that operates on the schedule
The schedule applies the commands after it ran all stages

Further work

Once we can operate on the schedule from systems, we may add more capabilities (removing systems, adding stages, ...)

alice-i-cecile · 2021-07-20T17:06:44Z

@Alainx277 could you comment in ~~#2358~~ #2373 to confirm that you're okay dual-licensing this under Apache + MIT?

alice-i-cecile · 2021-07-20T17:06:51Z

bors try

bors · 2021-07-20T17:08:54Z

try

Build failed:

build (stable, ubuntu-latest)

mockersf · 2021-07-20T17:35:37Z

@Alainx277 could you comment in #2358 to confirm that you're okay dual-licensing this under Apache + MIT?

it's #2373

crates/bevy_ecs/src/system/system_param.rs

crates/bevy_ecs/src/schedule/commands.rs

alice-i-cecile

This is a cool bit of functionality, and the code quality is solid. Eventually we'll want a complete "dynamic_schedule.rs" example, but I'm not sure that's useful with just the one function.

Architecturally, I'm worried about reinventing the wheel and leaky abstractions prominent in the new run_exclusive function that takes in schedule_commands explicitly. I would prefer if we either:

Extended the Command trait to also take a &mut Schedule in its write method, and then just allow commands to mutate the schedule.
Used the same mechanisms as Commands to create a dedicated ScheduleCommands.

Either way, we'd need to update apply_buffers to also operate on the schedule. Exclusive systems should also probably have the ability to modify the schedule.

crates/bevy_ecs/src/schedule/commands.rs

alice-i-cecile · 2021-07-20T19:30:52Z

bors try

bors · 2021-07-20T19:33:04Z

try

Build failed:

build (nightly, ubuntu-latest)

mockersf · 2021-07-20T19:35:09Z

CI failure for unrelated nightly issue

Alainx277 · 2021-07-20T20:14:34Z

Extended the Command trait to also take a &mut Schedule in its write method, and then just allow commands to mutate the schedule.

We cannot get a mutable reference to Schedule while running in a stage, as we already mutably borrow the Stage from Schedule.

Used the same mechanisms as Commands to create a dedicated ScheduleCommands.

I'm not sure what you mean by this? There already is a ScheduleCommands as a system parameter.

Either way, we'd need to update apply_buffers to also operate on the schedule. Exclusive systems should also probably have the ability to modify the schedule.

Same problem as with 1.

mockersf · 2021-07-20T20:30:40Z

Implementation looks good 👍

I dislike the name ScheduleCommand as it's ambiguous:

is it to schedule a command?
is it to issue a command to the schedule?

I think SchedulerCommand would lift the ambiguity, what do you think?

Also, for exclusive systems:

this becomes the only parameter that can't be obtained from world. while it makes sense, it's a change from other parameters
it adds a cost for every exclusive system, even if not issuing a ScheduleCommand

The PR could do with more doc comments and an example, that can be added in a followup

Alainx277 · 2021-07-20T21:19:31Z

I think SchedulerCommand would lift the ambiguity, what do you think?

Agreed!

it adds a cost for every exclusive system, even if not issuing a ScheduleCommand

I'm not sure how we could reduce the impact, any suggestions?

TheRawMeatball · 2021-07-22T15:49:35Z

bors try

bors · 2021-07-22T15:51:18Z

try

Build failed:

build (stable, ubuntu-latest)

TheRawMeatball · 2021-07-22T15:54:01Z

bors try

bors · 2021-07-22T15:57:49Z

try

Build failed:

build (stable, ubuntu-latest)

TheRawMeatball · 2021-07-22T16:37:53Z

bors try

bors · 2021-07-22T16:55:19Z

try

Build succeeded:

crates/bevy_ecs/src/system/commands/mod.rs

mockersf · 2021-07-24T11:44:23Z

the example crashes for me after around 20 seconds, moving asset loading from the added system to the main system fixes that

Ratysz

Still not sure about some things.

Scheduler* or Schedule*? I really don't like having "scheduler" anywhere in the docs or API, it muddies the waters wrt how things are named and working under the hood. We have a schedule that describes the systems graph, and an executor that executes the systems graph. A "scheduler" sounds to me like something that creates or modifies the graph - which would be fitting here, but we don't have a dedicated command-accepting abstraction for that: the schedule modifies itself.
The queue is a field of World and not an optional resource. This mingles concerns of World and Schedule - not catastrophically, but it will be grating for standalone bevy_ecs users that don't use the schedule abstraction.

This is not a call to action, I'd like to discuss these points first.

Nilirad · 2021-07-28T08:23:38Z

I agree on point 1. The word "scheduler" gives the idea of an agent that creates or modifies a schedule. At this point of time the "scheduler" is actually the programmer, who specifies how systems are ordered and if/when they should run. It is true that the Schedule does some kind of scheduling job, but always in the limits of what the programmer dictated.

Alainx277 · 2022-12-05T09:23:58Z

This PR is obsoleted by the new stageless scheduler.

Add ScheduleCommands

cfd590c

github-actions bot added S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 20, 2021

bors bot added a commit that referenced this pull request Jul 20, 2021

Try #2507:

0e82323

alice-i-cecile reviewed Jul 20, 2021

View reviewed changes

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 20, 2021

View reviewed changes

crates/bevy_ecs/src/schedule/commands.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jul 20, 2021

View reviewed changes

crates/bevy_ecs/src/schedule/commands.rs Outdated Show resolved Hide resolved

Fix formatting

e5ffab9

bors bot added a commit that referenced this pull request Jul 20, 2021

Try #2507:

50e871e

mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 20, 2021

Remove warning in impl_system_param_tuple

0f47181

Alainx277 added 3 commits July 20, 2021 22:49

Impl IntoSystemDescriptor for SystemDescriptor

037ade3

Store SystemDescriptor in InsertSystem

35df178

Rename ScheduleCommand to SchedulerCommand

be00194

Alainx277 added 2 commits July 22, 2021 11:53

Improve doctest for insert_system

8d4f86b

Add dynamic schedule example

8f76330

bors bot added a commit that referenced this pull request Jul 22, 2021

Try #2507:

0cfb567

Fix formatting

b67050b

bors bot added a commit that referenced this pull request Jul 22, 2021

Try #2507:

adf03f4

Satisfy clippy

98cb617

bors bot added a commit that referenced this pull request Jul 22, 2021

Try #2507:

a3c0c05

cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021

mockersf reviewed Jul 24, 2021

View reviewed changes

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved

mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021

Alainx277 added 2 commits July 25, 2021 13:07

Reuse material in dynamic_schedule example

b896bcd

Improve insert_system example

c3d1be2

mockersf approved these changes Jul 25, 2021

View reviewed changes

alice-i-cecile approved these changes Jul 25, 2021

View reviewed changes

Ratysz reviewed Jul 28, 2021

View reviewed changes

alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Sep 22, 2021

alice-i-cecile mentioned this pull request Oct 20, 2021

Dynamic Queries bevyengine/rfcs#37

Closed

alice-i-cecile mentioned this pull request Dec 12, 2021

API for dynamic management of systems (adding and removing at runtime) #279

Closed

alice-i-cecile added the X-Needs-SME This type of work requires an SME to approve it. label Apr 22, 2022

zicklag mentioned this pull request Aug 8, 2022

[Feature] Runtime Dynamic Components and Systems ( To Support Scripting Initiatives ) #142

Closed

Alainx277 closed this Dec 5, 2022

Uh oh!

Conversation

Alainx277 commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Further work

Uh oh!

alice-i-cecile commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jul 20, 2021

Uh oh!

bors bot commented Jul 20, 2021

try

Uh oh!

mockersf commented Jul 20, 2021

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alice-i-cecile commented Jul 20, 2021

Uh oh!

bors bot commented Jul 20, 2021

try

Uh oh!

mockersf commented Jul 20, 2021

Uh oh!

Alainx277 commented Jul 20, 2021

Uh oh!

mockersf commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alainx277 commented Jul 20, 2021

Uh oh!

TheRawMeatball commented Jul 22, 2021

Uh oh!

bors bot commented Jul 22, 2021

try

Uh oh!

TheRawMeatball commented Jul 22, 2021

Uh oh!

bors bot commented Jul 22, 2021

try

Uh oh!

TheRawMeatball commented Jul 22, 2021

Uh oh!

bors bot commented Jul 22, 2021

try

Uh oh!

Uh oh!

mockersf commented Jul 24, 2021

Uh oh!

Ratysz left a comment

Choose a reason for hiding this comment

Uh oh!

Nilirad commented Jul 28, 2021

Uh oh!

Alainx277 commented Dec 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Alainx277 commented Jul 20, 2021 •

edited

Loading

alice-i-cecile commented Jul 20, 2021 •

edited

Loading

mockersf commented Jul 20, 2021 •

edited

Loading