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

Possible fix for #1056 #1328

Closed

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Jan 27, 2021

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
Copy link
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
Copy link
Contributor

rmsc commented Feb 23, 2021

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
Copy link
Contributor

rmsc commented Feb 23, 2021

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
Copy link
Contributor Author

rparrett commented Mar 2, 2021

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
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
Development

Successfully merging this pull request may close these issues.

3 participants