Use schedule_scope instead of a manual (broken) hokey-pokey#23735
Use schedule_scope instead of a manual (broken) hokey-pokey#23735andriyDev wants to merge 3 commits intobevyengine:mainfrom
schedule_scope instead of a manual (broken) hokey-pokey#23735Conversation
andriyDev
added
C-Bug
urben1680
left a comment
urben1680
left a comment
There was a problem hiding this comment.
None of my notes are really blockers so this is an approve.
| let result = world.schedule_scope(label, |world, schedule| schedule.initialize(world)); | ||
| let Some(build_metadata) = result? else { | ||
| return Err( | ||
| "The schedule has already been built, so we can't collect its system data".into(), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Probably a matter of taste but I think this is a bit nicer:
let build_metadata = world
.schedule_scope(label, |world, schedule| schedule.initialize(world))?
.ok_or("The schedule has already been built, so we can't collect its system data")?;Not using a commitable suggestion here because I am unsure if the compiler is happy with that. If you do need an .into() on the str, you would want to use ok_or_else instead.
| let labels = schedules | ||
| .iter() | ||
| .map(|schedule| schedule.1.label()) | ||
| .collect::<Vec<_>>(); | ||
| let mut label_to_build_metadata = HashMap::new(); |
There was a problem hiding this comment.
I hope there is no cursed situation where initializing schedules creates new schedules as their labels are not included here yet. 🙃 Or equally cursed, remove schedules that would force you to use try_schedule_scope.
There was a problem hiding this comment.
I don't think that's possible. Schedule::initialize is only about initializing the graph data structure, not the user systems or anything. So unless bevy_ecs itself does something evil, this should be safe. I don't think we need to defend against this.
There was a problem hiding this comment.
But the docs say the systems get initialized here, so you could do funky stuff on custom System/Systemparam/WorldQuery impls.
There was a problem hiding this comment.
Oh you're totally right! My mistake!
Maybe it is just better to use try_schedule_scope just in case...
There was a problem hiding this comment.
If custom impl actually do add a new schedule, it is not unsafe if it does not get initialized here, right? I would shrug it off as totally unrealistic to happen, but when it gets to UB it might be worth a second thought.
There was a problem hiding this comment.
No it's not unsafe, but it might panic. That's all. Not horrific, but not ideal.
kfc35
left a comment
kfc35
left a comment
There was a problem hiding this comment.
This looks good to me
I can’t speak to whether systems creating/removing schedules is something we should be mindful of… I’m leaning towards no but I’m open to being corrected
kfc35
added
S-Ready-For-Final-Review
3 participants
Objective
Schedulesresource.Solution
schedule_scopewhich does all the business of hokey-pokey, and this currently handles errors for us!Testing