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

add try_ctx_for_window function #20

Merged
merged 1 commit into from
May 3, 2021

Conversation

jakobhellermann
Copy link
Contributor

This is necessary for bevy-inspector-egui to support multiple windows. Could you maybe release a small non-breaking version so that I can implement this without waiting for the next big release?

@mvlabat
Copy link
Owner

mvlabat commented May 2, 2021

What if we just change ctx_for_window to return an option? I'm personally fine with making a new minor release with this breaking change. I think it's easier to do so rather than making a patch release just to remove the try flavour later.

@jakobhellermann
Copy link
Contributor Author

In most cases ctx_for_window would be unwrapped if it returned an option. When creating a new window and doing .add_system_set(SystemSet::on_update(AppState::Done).with_system(ui_second_window.system())) the UI system will only run when the window exists.

My use case is for adding a ui system to a Plugin which runs even if the window is not set up yet.

@jakobhellermann
Copy link
Contributor Author

I think having both methods is fine, because the panicking version will be used the most, and the try_ version exists as a fallback for the more remaining cases. Same with vec[idx] vs vec.get(idx).

@mvlabat
Copy link
Owner

mvlabat commented May 3, 2021

Sounds fair. And thanks for the PR, I'll release the new version today.

@mvlabat mvlabat merged commit 4f4fd49 into mvlabat:main May 3, 2021
@jakobhellermann jakobhellermann deleted the try-get-context branch May 9, 2021 11:14
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.

None yet

2 participants