-
Notifications
You must be signed in to change notification settings - Fork 2.1k
buffer update opt: Integrate UboManager into the engine #9397
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
base: main
Are you sure you want to change the base?
Conversation
…)" This reverts commit 49c4a5d.
# Conflicts: # filament/src/details/MaterialInstance.cpp
filament/src/details/Renderer.cpp
Outdated
| std::optional<UboManager>& uboManager = engine.getUboManager(); | ||
| assert_invariant(uboManager.has_value()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to return the optional and check isUboBatchingEnabled(). I would keep the isUboBatchingEnabled() check and just write engine.getUboManager()->endFrame()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| FMaterialInstance::FMaterialInstance(FEngine& engine, | ||
| FMaterialInstance const* other, const char* name) | ||
| FMaterialInstance const* other, const char* name, bool useUboBatching) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it necessary to add the bool here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess sometimes there is case that we need to create a new MI from a defaultMaterial instance.
Since defaultMaterial instance needs to be committed right after it is created, I exclude it from using UBO batching, but for the MI that is created from the defaultMaterial I think they should have the ability to be using UBO batching.
I'm not sure if this case would really happen though, any thought?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is uncommon - to have many instances of the default material. I think it's ok to not offer that option for duplicate() and the duplicate constructor of FMaterialInstance.
But defer to @pixelflinger
Sidenote:
I think to add this, you'd also have to expose the option in the public MaterialInstance::duplicate()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me think more about this and get back to you.
# Conflicts: # filament/src/details/MaterialInstance.cpp
# Conflicts: # NEW_RELEASE_NOTES.md
pixelflinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized, did you handle FRenderer::renderStandaloneView? I don't think you did.
I did (in the latest commit). I also tested with a custom app which calls Note: I feel like it makes more sense to add an |
To focus on the integration logic added in this PR, please ignore
BufferAllocatorandUboManagerand their related tests. They were introduced in the base PR (#9331).Changes
UboManager. It initializes it, calls itsbeginFrame/endFramemethods, and manages MaterialInstance slot retirement on destruction.useUboBatchingflag.commit()now writes data to the shared buffer viauboManager->updateSlot.use()now binds the shared buffer using its dynamic offset.createInstance()methods are updated to pass theuseUboBatchingflag to the MaterialInstance.mi->commit(driver)are updated tomi->commit(driver, mEngine.getUboManager())to pass the manager.endFrame(), it now notifies the UboManager to create a fence for GPU synchronization.DYNAMIC_OFFSETflag for the material's uniform buffer, so that it could update the offset without recreating the descriptor set.