Skip to content

Conversation

Alainx277
Copy link

@Alainx277 Alainx277 commented Jul 20, 2021

Objective

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, ...)

@github-actions 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
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work and removed 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
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 20, 2021

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

@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 20, 2021
@bors
Copy link
Contributor

bors bot commented Jul 20, 2021

try

Build failed:

@mockersf
Copy link
Member

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

it's #2373

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Extended the Command trait to also take a &mut Schedule in its write method, and then just allow commands to mutate the schedule.
  2. 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.

@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 20, 2021
@bors
Copy link
Contributor

bors bot commented Jul 20, 2021

try

Build failed:

@mockersf 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
@mockersf
Copy link
Member

CI failure for unrelated nightly issue

@Alainx277
Copy link
Author

  1. 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.

  1. 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
Copy link
Member

mockersf commented Jul 20, 2021

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
Copy link
Author

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
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 22, 2021
@bors
Copy link
Contributor

bors bot commented Jul 22, 2021

try

Build failed:

@TheRawMeatball
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 22, 2021
@bors
Copy link
Contributor

bors bot commented Jul 22, 2021

try

Build failed:

@TheRawMeatball
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 22, 2021
@bors
Copy link
Contributor

bors bot commented Jul 22, 2021

@cart 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
Copy link
Member

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

@mockersf 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
Copy link
Contributor

@Ratysz Ratysz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure about some things.

  1. 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.

  2. 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
Copy link
Contributor

Nilirad commented Jul 28, 2021

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.

@alice-i-cecile 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 alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
@Alainx277
Copy link
Author

This PR is obsoleted by the new stageless scheduler.

@Alainx277 Alainx277 closed this Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants