Decouple gfx-replay player from scene graph; integrate the batch renderer into it#1947
Conversation
0211bd6 to
692a3e6
Compare
1f85c05 to
6116a6f
Compare
mosra
left a comment
There was a problem hiding this comment.
Overall, this is exactly how I wanted this to be. A bunch of minor comments.
0bfc210 to
1104cca
Compare
I don't see any practical use for it there.
Pass --once to the app to repeat just once. Useful for benchmarking the renderer without the JSON parsing overhead, which is apparently significantly higher than all operations the renderer needs to do :D
dbc2668 to
57f74bd
Compare
bb7e787 to
be2e658
Compare
Because *imagine* how it would be if we could put comments next to the actual option we're adding. IMAGINE.
Not sure how important is to have this particular thing working there, so it's just an unreachable assert.
e85af83 to
a44a3a1
Compare
It was crashing because the python bindings for ReplayManager::readKeyframesFromFile() returned a Player but didn't tell Python the Player depends on the PlayerImplementation stored inside the ReplayManager. Which would involve some additional bookkeeping in the bindings, and ... everything else is using a shared_ptr anyway, so trying to avoid the unnecessary overhead in just a single place of all is like a drop in the ocean. Doesn't crash anymore with this change.
a44a3a1 to
a382d41
Compare
| environmentGridSize(cfg.numEnvironments)); | ||
| if ((standalone_ = cfg.standalone)) | ||
| renderer_.emplace<gfx_batch::RendererStandalone>( | ||
| configuration, gfx_batch::RendererStandaloneConfiguration{}); |
There was a problem hiding this comment.
We can carry the GPU ID here:
renderer_.emplace<gfx_batch::RendererStandalone>(
configuration, gfx_batch::RendererStandaloneConfiguration{}
.setCudaDevice(cfg.gpuDeviceId));
There was a problem hiding this comment.
Good point, I'll add this -- seems important.
There was a problem hiding this comment.
Ah well, the default ReplayRendererConfiguration::gpuDeviceId is 0, but 0 means the first CUDA device -- i.e., if you have no NV cards (or the NV card disconnected as in my case), it would fail to create the context.
The default should be -1 I think, i.e. picking whatever the primary GPU is, so it's not restricted to work only on NVidias. Can I change the default to -1?
There was a problem hiding this comment.
Oh that's right. -1 by default makes sense then.
There was a problem hiding this comment.
Not sure if I want to go through the whole CI process again -- we want to merge ASAP -- could you add this in the followup PR? :)
There was a problem hiding this comment.
The default ReplayRendererConfiguration::gpuDeviceId is 0 because the default SimulatorConfiguration::gpuDeviceId is 0. @0mdc , if you're making a change, consider making it in both places.
eundersander
left a comment
There was a problem hiding this comment.
I left some comments.
| environmentGridSize(cfg.numEnvironments)); | ||
| if ((standalone_ = cfg.standalone)) | ||
| renderer_.emplace<gfx_batch::RendererStandalone>( | ||
| configuration, gfx_batch::RendererStandaloneConfiguration{}); |
There was a problem hiding this comment.
The default ReplayRendererConfiguration::gpuDeviceId is 0 because the default SimulatorConfiguration::gpuDeviceId is 0. @0mdc , if you're making a change, consider making it in both places.
9c32b26 to
cadfa0f
Compare
22a5ba5 to
098e914
Compare
…ble. The comment stays there though, for historical records.
Motivation and Context
Decouples
gfx::replay::PlayerfromSceneNodein order to allow the batch renderer to be integrated, and integrates the batch renderer. Gradually taken over by @mosra.NodeHandletype, which can be cast into eitherSceneNode*or an index, depending on the implementation.gfx::replay::Playerimplementation into a pure virtual interfaceAbstractReplayRendererclass, moving the originalReplayBatchRendererto just aReplayRendererReplayBatchRendererclassreplayerutility, to which you can pass a set of JSON replay files, which then get played back--classicto it to switch to the classic one--profileto print frame profiling info to the outputAbstractReplayRenderer(which would be a no-op for the classic one), hooking that up into thereplayerShowing batch renderer frame stats in the profiling outputpostponed, would require a change in Magnum'sFrameProfilerThe following shows running
./build/src/utils/replayer/replayer \ ./data/RE_policy.replay.json \ ./data/R_human_table_cabclosed_simplified.replay.fixed.json \ ./data/v3_sc0_staging_00.replay.json \ --preload ./data/composite-replicacad.gltf \ --preload ./data/composite-ycb.gltf \ --preload ./data/composite-fetch.gltfwith replay files taken from here (with the
.fixed.jsonbeing patched to reference a real file instead of acubeSolidplaceholder) and composite files taken from this and this Slack thread:How Has This Been Tested
CI is not passing because the PR is done from a forked repo.I found a hack which makes it 🟢No idea what's up with the CI. It used to pass, now it fails on
AttributeError: module 'torch._C' has no attribute '_cuda_setDevice'again.