buffer update opt: Some optimizations - #9438
Conversation
That seems like a good change. I would assume that adding/removing materialinstance is a low frequency operation. the extra memory seems okay, but we need to make sure those lists are updated when a MaterialInstance is destroyed. |
| if (!BufferAllocator::isValid(id)) | ||
| return; |
There was a problem hiding this comment.
what does that mean here that the isValid() could be false? Shouldn't we remove the material instance from the list anyway? should this be an assert? or is this a valid case?
There was a problem hiding this comment.
If you create a material instance and destroy it immediately, the id here would be invalid (though it should be rare).
And you're right, we still need to remove it from the list, but should not retire this slot since the id is invalid. I moved this check after the erase. Thanks for the catch!
Changes
allocateOnDemandwhen previous allocation has already failedUboManagernow internally manages material instances which have batching enabledallocateOnDemanduses the new internal listI'd like to know your thoughts about managing a separate list for batching-enabled material instances.
The benefit of this is that the logic inside functions become clearer. For
allocateOnDemand, the logic becomes simpler and potentially more efficient as well since it only traverses required material instances. The downside of this is that it requires additional memory to maintain a separate list.