-
Notifications
You must be signed in to change notification settings - Fork 7
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
Replace InMemoryAssetResolver with DynamicTextureProvider #252
Conversation
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.
Looks good. One open question.
@@ -78,6 +78,7 @@ set(CARB_ROOT "${NVIDIA_BUILD_DIR}/target-deps/carb_sdk_plugins") | |||
set(KIT_SDK_ROOT "${NVIDIA_BUILD_DIR}/target-deps/kit-sdk") | |||
set(PYBIND11_ROOT "${NVIDIA_BUILD_DIR}/target-deps/pybind11") | |||
set(CUDA_ROOT "${NVIDIA_BUILD_DIR}/target-deps/cuda") | |||
set(RTX_PLUGINS_ROOT "${NVIDIA_BUILD_DIR}/target-deps/rtx_plugins") |
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.
As discussed, please raise this up to Nvidia so we can understand if this is an expected dependency.
auto& referenceCount = accessor->second->_referenceCount; | ||
referenceCount--; | ||
if (referenceCount == 0) { |
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.
As long as found
is held it blocks other threads, correct? I'm concerned about if there's a case where we could run into a situation where reference count could go negative.
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 believe accessor
acts as a lock guard, so we shouldn't see any race conditions
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.
Sounds good. This is good to go once you fix the Windows builds. I'll give it a quick once over when that's done.
269c63d
to
318a3ae
Compare
318a3ae
to
ead6170
Compare
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.
🚀
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.
As discussed offline. Blocking this due to the perceived performance issues and warning spam. We'll revisit this with either the next Kit release or if we get a workaround.
56f3c76
to
1429d37
Compare
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.
Good to go.
Fixes #116
Removes
InMemoryAssetResolver
and uses Omniverse's built-inDynamicTextureProvider
instead. This has several benefits:DynamicTextureProvider
. That can be a follow up PR./rtx-transient/resourcemanager/enableTextureStreaming = false
workaround from InMemoryAssetResolver plugin not being called #95This doesn't fix the crashes / hangs we've noticed. At least not all of them. I can still trigger hangs by reloading agi.usda repeatedly.
One potential blocker is that each time a texture is loaded a warning gets printed. I reached out to the Nvidia team about this.
To do:
Footnotes
Though a restart is still required to register Cesium USD schemas ↩