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

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

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Jan 4, 2021

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

cart commented Jan 7, 2021

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

cart commented Jan 7, 2021

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

@mockersf
Copy link
Member Author

mockersf commented Jan 7, 2021

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

mockersf commented Jan 7, 2021

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

cart commented Jan 7, 2021

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

cart commented Jan 11, 2021

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