Skip to content

use largest RenderResources instead of first to initialize uniform buffers - #1208

Merged
cart merged 1 commit into
bevyengine:masterfrom
mockersf:biggest-resource
Jan 11, 2021
Merged

cart merged 1 commit into
bevyengine:masterfrom
mockersf:biggest-resource

Conversation

@mockersf

@mockersf mockersf commented Jan 4, 2021

Copy link
Copy Markdown
Member

Fixes #1056

When using two texture atlas with different sprite counts, the buffers were sometimes initialized with the smallest, causing a panic when sending the largest.
See example #1056 (comment) for a reproducer, with one atlas of 8 (2 by 4) and the other of 177 (3 by 59) sprites.

@cart

cart commented Jan 7, 2021

Copy link
Copy Markdown
Member

This resolves the issue mentioned, but its a bit of a hack because we're now allocating space for the largest sprite count across all atlases. The UniformBufferArray abstraction was written for "dynamic uniforms", which must have the exact same size for each item in the array. The code is breaking for non-dynamic uniforms (specifically buffer/storage uniforms, which can be arrays of any size).

Clearly I bungled non-dynamic uniform support 😄

I think the "correct" fix would be to allocate exactly the right amount of space in the staging buffer, probably as a separate step for non-dynamic uniforms outside of UniformBufferArray. We should probably only use UniformBufferArray when dynamic = true.

On the other hand, thats a lot of work and the current system has a number of flaws / limitations. Maybe its worth scrapping the whole thing and building the ideal system (whatever that looks like).

@cart

cart commented Jan 7, 2021

Copy link
Copy Markdown
Member

I'm cool with merging this as-is, provided we have plans to follow up / record that as an issue.

@mockersf

mockersf commented Jan 7, 2021 •

Copy link
Copy Markdown
Member Author

its a bit of a hack because we're now allocating space for the largest sprite count across all atlases

Yup, I figured as much... but it's a part of the code I'm not (yet) completly comfortable with and it took me already long enough to track it to this 😄
Still I think it's a "good enough" hack, as currently it's randomly either allocating space for the largest, or crashing. With this it's consistently allocating more space.

@mockersf

mockersf commented Jan 7, 2021

Copy link
Copy Markdown
Member Author

I'm cool with merging this as-is, provided we have plans to follow up / record that as an issue.

I can open an issue copy pasting your comment above, but I'll finish googling some of the things inside first !

@cart

cart commented Jan 7, 2021 •

Copy link
Copy Markdown
Member

Cool that all sounds good to me. If nobody else gets around to it, I'll probably pick up the "high level data binding" rewrite soon-ish. The current impl even confuses me and I wrote the dang thing / understand the concepts!

@darthdeus

Copy link
Copy Markdown
Contributor

How come more people aren't experiencing this issue though? Do people just build one big texture atlas, or I mean should we be doing that?

@cart

cart commented Jan 11, 2021

Copy link
Copy Markdown
Member

There are plenty of legitimate reasons to use multiple atlases (ex: staying under device texture size limits if you have many sprites or your sprites are large. logical separation, etc). We should support this scenario 😄

I have a feeling that more people aren't hitting it because in most cases only one atlas is needed and thats "easier".

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.

Error loading two different texture atlases back to back

3 participants