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

Fix bevymark crash #7404

Closed
wants to merge 4 commits into from
Closed

Fix bevymark crash #7404

wants to merge 4 commits into from

Conversation

opstic
Copy link
Contributor

@opstic opstic commented Jan 28, 2023

Objective

Fixes #7403

Solution

  • Replace windows.single() with windows.get_single()? then use system piping to return early.

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.

I would use if let Some(window) instead here: it's generally clearer IMO.

@alice-i-cecile
Copy link
Member

You could also use the new pipe adaptors and ?: I'd be happy with either option.

@opstic
Copy link
Contributor Author

opstic commented Jan 28, 2023

@alice-i-cecile Done, it's now using if let to set the window variable.

@alice-i-cecile
Copy link
Member

Ah: I meant to wrap the entire block below that relies on window existing into the if let statement :)

In rust, I would use the ? operator (and return a Result) to convey that early return pattern more idiomatically.

@opstic
Copy link
Contributor Author

opstic commented Jan 28, 2023

@alice-i-cecile Done, now it's using system piping.

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.

Very nice! It's good to demo this pattern in more examples too.

@alice-i-cecile alice-i-cecile added C-Examples An addition or correction to our examples A-Windowing Platform-agnostic interface layer to run your app in labels Jan 28, 2023
@mockersf
Copy link
Member

mockersf commented Jan 28, 2023

That should not happen, the system you fixed is in stage Update and getting the window in that stage should not panic (unless the developer willingly made a way to not have windows). Ignoring that failure is not the right fix, we should find the root cause. Maybe an interaction with the fact that the system is in a time step.

I would wait for the new scheduler PR (#7267) to be merged (which changes a lot of that), and then try to reproduce the crash with it, and if that's still present try to find the root cause

@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR labels Jan 28, 2023
@opstic
Copy link
Contributor Author

opstic commented Jan 28, 2023

Alright, so should I close this?

@alice-i-cecile
Copy link
Member

Yeah, let's close this out for now and leave the issue open :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Examples An addition or correction to our examples S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bevymark sometimes panics with "Called Result::unwrap() on an Err value"
3 participants