Skip to content

Possible fix for #1056 - #1328

Closed
rparrett wants to merge 2 commits into
bevyengine:mainfrom
rparrett:maybe-fix-staging-buffer-panic
Closed

Possible fix for #1056#1328
rparrett wants to merge 2 commits into
bevyengine:mainfrom
rparrett:maybe-fix-staging-buffer-panic

Conversation

@rparrett

@rparrett rparrett commented Jan 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

I don't want to pretend to be able to explain what's going on here.

This is definitely a hackjob, but it might be the case that it's a hackjob that's well aligned with another recent hack (#1208) around the same code. It's entirely possible that I'm just accidentally fixing this by oversizing the staging buffer or something.

I'm tossing this here so that cart or someone else with experience with this code might determine whether or not it's doing anything useful. I'm happy to just keep my fork patched locally so that I can write game code again for a while if it's no good.

This information is also buried in #1056 but I'll summarize here:

This fixes a panic that occurs in both of these cases:

https://github.com/rparrett/taipo/tree/spritesheet-panic-two (game will play itself until it panics when the crabs start dying)
https://github.com/rparrett/bevy-test/tree/render-panic (just displays a chunk of text one letter at a time)

This seems to be related to font atlases and tweaking font sizes affects time-to-panic. I'm on macOS with a high DPI display. So YMMV.

@rparrett
rparrett force-pushed the maybe-fix-staging-buffer-panic branch 2 times, most recently from 0791208 to ef00598 Compare February 10, 2021 00:29
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior help wanted A-Rendering Drawing game state to the screen and removed help wanted labels Feb 17, 2021
@rparrett
rparrett force-pushed the maybe-fix-staging-buffer-panic branch from ef00598 to 182a7ec Compare February 19, 2021 00:55
Base automatically changed from master to main February 19, 2021 20:44
@rparrett

rparrett commented Feb 21, 2021

Copy link
Copy Markdown
Contributor Author

Sadly, this PR doesn't seem to completely fix the problem. However I am only seeing the panic very intermittently now and can't reliably reproduce it yet.

@rmsc

rmsc commented Feb 23, 2021

Copy link
Copy Markdown
Contributor

Sadly, this PR doesn't seem to completely fix the problem. However I am only seeing the panic very intermittently now and can't reliably reproduce it yet.

I was able to reliably reproduce this using your render-panic example, and got curious enough to poke around. What I found is interesting. Just before a crash happens, for example:

  • prepare_uniform_buffers() reports a required size of 17656
  • write_uniform_buffers() panics while trying to write to 17656, while the slice length is only 17632

To me this tells me that the buffer isn't being properly resized. Will investigate further.

@rmsc

rmsc commented Feb 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

The problem is that set_required_staging_buffer_size_to_max() is destroying the computed required_staging_buffer_size, and so preventing resize_staging_buffer() from resizing the buffer.

I'm definitely think this is the wrong way to fix this. I've tried an alternative fix (#1509), could you please have a look?

@rparrett

rparrett commented Mar 2, 2021

Copy link
Copy Markdown
Contributor Author

Closing in favor of #1509

@rparrett rparrett closed this Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rparrett @rmsc @alice-i-cecile