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

with_children takes a FnMut which prevents some use cases #531

Closed
HyperLightKitsune opened this issue Sep 20, 2020 · 4 comments · Fixed by #535
Closed

with_children takes a FnMut which prevents some use cases #531

HyperLightKitsune opened this issue Sep 20, 2020 · 4 comments · Fixed by #535
Assignees
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible

Comments

@HyperLightKitsune
Copy link
Contributor

FnMut prevents transferring ownership.

For example, the following will not compile.

fn spawn(mut commands: Commands, text: String) {
    commands.spawn((0,)).with_children(move |parent| parent.spawn((text,));
}

This is because FnMut can be called multiple times. I believe Commands will only call this function once so it shouldn't be a problem.

This should be a simple change. I'm pretty sure we can just change the input from a FnMut to a FnOnce.

I am completely willing to implement this myself.

@karroffel karroffel added C-Code-Quality A section of code that is hard to understand or change A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Sep 20, 2020
@karroffel
Copy link
Contributor

I think you're free to work on it and open a PR! I can assign you so nobody else does duplicate work. If you don't want to/can implement that just let me know and I remove you again as the asignee! 💙

@HyperLightKitsune
Copy link
Contributor Author

I am sure there are other functions like this (for example for_current_entity). Should I do it for all of them? Should I open different PRs for each one?

@karroffel
Copy link
Contributor

You can rename the PR to something like "Use FnOnce where possible" maybe? That way it works for more cases too.

@HyperLightKitsune
Copy link
Contributor Author

Ok, will do.

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-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants