Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix] add_stage now checks Stage existence #1346

Merged
merged 3 commits into from
Jan 30, 2021

Conversation

Telzhaak
Copy link
Contributor

What's the bug?

add_stage never checked whether a given Stage-name already exists (which extends to with_stage())

pub fn add_stage<S: Stage>(&mut self, name: &str, stage: S) -> &mut Self {
self.stage_order.push(name.to_string());
self.stages.insert(name.to_string(), Box::new(stage));
self
}

instead simply

  1. replacing the previously held Stage in Schedule.stages (effectively removing all systems/schedulers/... attached to that old Stage)
  2. Appending the Stage-name to the vector Schedule.stage_order, while not removing the already existing one, creating a duplicate

How is it fixed?

This has been fixed using the same method the other two functions add_stage_after and add_stage_before use:

if self.stages.get(name).is_some() {
    panic!("Stage already exists: {}.", name);
}

This also raises the question:

  • Should these functions even panic when a Stage-name is already taken, or should some sort of Error-flow be implemented to allow handling it?

Why does it matter?

Since this bug effectively changed system-execution, it could have caused some interesting and hard to debug problems.
Especially later on, when other Stages are added in relation to this one. add_stage is supposed to place a Stage at the end of execution-order at the time of calling (Vec::push()). Thus anything added via e.g. add_stage_before(this_broken_stage) would be expected to execute as e.g. second-last. However add_stage_before determines positioning via:

let target_index = self
  .stage_order
  .iter()
  .enumerate()
  .find(|(_i, stage_name)| *stage_name == target)
  .map(|(i, _)| i)
  .unwrap_or_else(|| panic!("Target stage does not exist: {}.", target))

This would return the index of the duplicate in Schedule.stage_order at the old position, earlier in the execution-order.

Telzhaak and others added 2 commits January 30, 2021 11:51
bjorn3 suggested this change to improve startup time

Co-authored-by: bjorn3 <[email protected]>
@Telzhaak
Copy link
Contributor Author

Telzhaak commented Jan 30, 2021

As @cart elaborated in the resolved conversation, panicking is the way to go for now. Error-handling being a discussion to be held in a different issue.

@bjorn3 's changes have been merged and additionally been applied to add_stage_before and add_stage_after, removing one HashMap-lookup each, to improve startup-times

@cart cart merged commit 61c9a40 into bevyengine:master Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants