-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[vulkan] Improve Vulkan RHI impl with lower overhead internal implementations #6912
Conversation
✅ Deploy Preview for docsite-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
lgtm! thank you
// It does not mark objects as used, and it does not free objects (destructor is | ||
// not called) | ||
template <class T> | ||
class SyncedPtrStableObjectList { |
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.
so to my understanding the the contained objects should be explicitly constructed/destructed?
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.
That's a good point. It seems like we should add a check that T
is a POD-ish type, which doesn't have fancy ctor/dtor. (Dtor is especially important here, IMO. Right now the implementation would require the caller to call ~T()
)
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.
Fixed it now, the the object list now handles ctor/dtor & uses RAII.
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.
nit: I suggest that we add some unit test on this (https://github.com/taichi-dev/taichi/tree/master/tests/cpp), given that this is a very low-level infra, and has certain behaviors that is not trivial (manually calling ~T()
, placement new, ...)
…ntations (taichi-dev#6912) Also added partial RHI support for Vulkan Dynamic Rendering, but there are some interactions and bugs to figure out between this and ImGUI, so the device creator will not actively enable this feature for now. Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Also added partial RHI support for Vulkan Dynamic Rendering, but there are some interactions and bugs to figure out between this and ImGUI, so the device creator will not actively enable this feature for now.