Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Decouple gfx-replay player from scene graph; integrate the batch renderer into it #1947

Merged
merged 56 commits into from
Dec 19, 2022
Merged

Decouple gfx-replay player from scene graph; integrate the batch renderer into it #1947

merged 56 commits into from
Dec 19, 2022

Conversation

0mdc
Copy link
Contributor

@0mdc 0mdc commented Nov 23, 2022 •
edited by mosra
Loading

Motivation and Context

Decouples gfx::replay::Player from SceneNode in order to allow the batch renderer to be integrated, and integrates the batch renderer. Gradually taken over by @mosra.

  • Introducing an opaque NodeHandle type, which can be cast into either SceneNode* or an index, depending on the implementation.
  • Moving gfx::replay::Player implementation into a pure virtual interface
  • Creating a base AbstractReplayRenderer class, moving the original ReplayBatchRenderer to just a ReplayRenderer
  • Creating a brand new ReplayBatchRenderer class
  • Adapting the test to run with both, expecting a ~matching output
  • Adding a GUI replayer utility, to which you can pass a set of JSON replay files, which then get played back
    • Uses the batch renderer by default, pass --classic to it to switch to the classic one
    • Use --profile to print frame profiling info to the output
  • Ability to preload composite files in AbstractReplayRenderer (which would be a no-op for the classic one), hooking that up into the replayer
  • Showing batch renderer frame stats in the profiling output postponed, would require a change in Magnum's FrameProfiler
  • Ability to repeat the replay files or not
  • "Resolve" the Emscripten build failure -- attempting to use the classic renderer in the gfx-replay pipeline under Emscripten will assert. I'm not sure what exactly to do there, and I'm not sure if such workflow is even something we want to support moving forward.
  • Resolving remaining essential TODOs, like the commented-out cleanup on Player destruction

The 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.gltf

with replay files taken from here (with the .fixed.json being patched to reference a real file instead of a cubeSolid placeholder) and composite files taken from this and this Slack thread:

replayer-2x

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.

@0mdc 0mdc requested review from mosra and eundersander November 23, 2022 20:09
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 23, 2022
@0mdc 0mdc changed the title [DRAFT] Decouple gfx-replay player from scene graph. Decouple gfx-replay player from scene graph. Nov 23, 2022
@0mdc 0mdc force-pushed the gfx-replay-scene-decoupling branch 3 times, most recently from 0211bd6 to 692a3e6 Compare November 23, 2022 22:33
@0mdc 0mdc force-pushed the gfx-replay-scene-decoupling branch from 1f85c05 to 6116a6f Compare November 24, 2022 15:23
mosra
mosra reviewed Nov 28, 2022
View reviewed changes
Copy link
Collaborator

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is exactly how I wanted this to be. A bunch of minor comments.

src/esp/gfx/replay/PlayerCallbacks.h Outdated Show resolved Hide resolved
src/esp/gfx/replay/PlayerCallbacks.h Outdated Show resolved Hide resolved
src/esp/scene/SceneNode.h Outdated Show resolved Hide resolved
src/esp/scene/SceneNode.h Outdated Show resolved Hide resolved
src/esp/scene/SceneNode.h Outdated Show resolved Hide resolved
src/tests/GfxReplayTest.cpp Outdated Show resolved Hide resolved
@0mdc 0mdc marked this pull request as ready for review November 28, 2022 16:23
@0mdc 0mdc force-pushed the gfx-replay-scene-decoupling branch from 0bfc210 to 1104cca Compare November 28, 2022 16:24
eundersander
eundersander reviewed Nov 29, 2022
View reviewed changes
Copy link
Contributor

@eundersander eundersander left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress! I left some comments.

src/esp/sim/Simulator.h Outdated Show resolved Hide resolved
src/esp/gfx/replay/PlayerCallbacks.h Outdated Show resolved Hide resolved
mosra added 4 commits December 12, 2022 16:55
@mosra
Don't build the replayer utility on Emscripten.
f0b96f8 
I don't see any practical use for it there.
@mosra
Fix a TODO related to crash on exit.
bf1d792
@mosra
Clean up, clarify TODOs, revert useless changes.
ad6da66
@mosra
replayer: repeat the replays by default.
57f74bd 
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
@mosra mosra force-pushed the gfx-replay-scene-decoupling branch from dbc2668 to 57f74bd Compare December 12, 2022 16:01
@mosra mosra force-pushed the gfx-replay-scene-decoupling branch from bb7e787 to be2e658 Compare December 12, 2022 16:11
mosra added 2 commits December 12, 2022 17:29
@mosra
Forgot to add the actual Clang Tidy suppression.
457d0cf 
Because *imagine* how it would be if we could put comments next to the
actual option we're adding. IMAGINE.
@mosra
Make the Emscripten build compile at least.
bb2edee 
Not sure how important is to have this particular thing working there,
so it's just an unreachable assert.
@mosra mosra force-pushed the gfx-replay-scene-decoupling branch 2 times, most recently from e85af83 to a44a3a1 Compare December 13, 2022 19:05
@mosra
Give up and use a shared_ptr for PlayerImplementation.
a382d41 
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.
@mosra mosra force-pushed the gfx-replay-scene-decoupling branch from a44a3a1 to a382d41 Compare December 13, 2022 19:17
0mdc
0mdc commented Dec 15, 2022
View reviewed changes
src/esp/sim/ReplayBatchRenderer.cpp Outdated
environmentGridSize(cfg.numEnvironments));
if ((standalone_ = cfg.standalone))
renderer_.emplace<gfx_batch::RendererStandalone>(
configuration, gfx_batch::RendererStandaloneConfiguration{});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can carry the GPU ID here:

renderer_.emplace<gfx_batch::RendererStandalone>(
        configuration, gfx_batch::RendererStandaloneConfiguration{}
          .setCudaDevice(cfg.gpuDeviceId));

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll add this -- seems important.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's right. -1 by default makes sense then.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
eundersander reviewed Dec 15, 2022
View reviewed changes
Copy link
Contributor

@eundersander eundersander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments.

src/esp/gfx/replay/Player.cpp Show resolved Hide resolved
src/esp/sim/ReplayBatchRenderer.h Outdated Show resolved Hide resolved
src/esp/sim/ReplayBatchRenderer.h Outdated Show resolved Hide resolved
src/esp/sim/ReplayBatchRenderer.cpp Outdated
environmentGridSize(cfg.numEnvironments));
if ((standalone_ = cfg.standalone))
renderer_.emplace<gfx_batch::RendererStandalone>(
configuration, gfx_batch::RendererStandaloneConfiguration{});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/esp/sim/ReplayBatchRenderer.h Outdated Show resolved Hide resolved
src/esp/sim/ReplayBatchRenderer.h Outdated Show resolved Hide resolved
src/utils/replayer/replayer.cpp Show resolved Hide resolved
@mosra mosra force-pushed the gfx-replay-scene-decoupling branch from 9c32b26 to cadfa0f Compare December 16, 2022 17:30
@mosra mosra force-pushed the gfx-replay-scene-decoupling branch from 22a5ba5 to 098e914 Compare December 16, 2022 18:15
@mosra mosra mentioned this pull request Dec 16, 2022
Merged
Skylion007
Skylion007 reviewed Dec 16, 2022
View reviewed changes
.clang-tidy Outdated Show resolved Hide resolved
@0mdc 0mdc merged commit 0b7e5eb into facebookresearch:main Dec 19, 2022
@mosra mosra mentioned this pull request Dec 20, 2022
Merged
11 tasks
@0mdc 0mdc deleted the gfx-replay-scene-decoupling branch June 14, 2023 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosra mosra mosra left review comments

@Skylion007 Skylion007 Skylion007 approved these changes

@eundersander eundersander eundersander left review comments

Assignees
No one assigned
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@0mdc @Skylion007 @mosra @eundersander @facebook-github-bot