[AO migration] async rendering by aclegg3 · Pull Request #1229 · facebookresearch/habitat-sim

aclegg3 · 2021-05-11T00:36:32Z

Motivation and Context

This PR introduces async rendering in a background thread allowing overlap of CPU physics simulation and GPU rendering operations resulting in a significant performance improvement. For training scenarios, this does result in delayed observation (the visual observations passed to the model will be one step behind the physical state) which is, however, analogous with sensor latency on physical systems.

Note: this feature is not enabled by default due to instability on some systems and potential for memory leaks if not properly used. See examples/tutorials/async_rendering.py for details.

Thanks @erikwijmans for building the vast majority of this feature.

How Has This Been Tested

Adds a python example with CI testing.

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

aclegg3 · 2021-05-11T00:38:28Z

@erikwijmans I'd appreciate your input on this PR since you created this feature. 🙂
@bigbike I think this falls into your area of expertise, I'd appreciate your thoughts on fitting this into the new/upcoming sensor rendering paradigm.

eundersander · 2021-05-13T20:53:07Z

+
+#include <atomic_wait.h>
+
+contended_t contention[256];


How about some comments here, and let's avoid unnamed magic numbers like 256 and 255.

Oops, didn't notice this is a dependency from Nvidia. Would be nice to document where exactly we got this (since it's not a submodule). For example, do they version this?

This is a reference implementation for cpp20's atomic wait, it isn't part of a library so that's why it's not a submodule (it isn't intended to be compiled by another project) and it is not versioned (as this is the only version).

eundersander

I still consider it an open question whether we should merge this to master. It would be nice if the PR description listed the known caveats/issues with this approach. (@aclegg3 mentioned that it was unsafe to reset the scene or something like that).

Anyway I left some comments.

eundersander · 2021-05-13T20:56:00Z

+
+#include <atomic_wait.h>
+
+contended_t contention[256];


Oops, didn't notice this is a dependency from Nvidia. Would be nice to document where exactly we got this (since it's not a submodule). For example, do they version this?

eundersander · 2021-05-13T20:56:34Z

+            self.drawAsync(visualSensor, sceneGraph, view,
+                           RenderCamera::Flags{flags});
+          },
+          R"(Draw given scene using the visual sensor)", "visualSensor"_a,


Need better comments for all these new bindings.

eundersander · 2021-05-13T20:57:02Z

      .def("reconfigure", &Simulator::reconfigure, "configuration"_a)
      .def("reset", &Simulator::reset)
-      .def("close", &Simulator::close)
+      .def("close", &Simulator::close, "destroy"_a = false)


Needs a comment. What is close versus close-and-destroy?

eundersander · 2021-05-13T20:59:32Z

 #include "esp/sensor/VisualSensor.h"
 #include "esp/sim/Simulator.h"

+#if !defined(CORRADE_TARGET_EMSCRIPTEN)


I'd prefer to see #ifdef ENABLE_ASYNC_RENDERING everywhere instead of !defined(CORRADE_TARGET_EMSCRIPTEN) everywhere. It would be more readable. And we may want to enable/disable this code based on other reasons in the future.

eundersander · 2021-05-13T21:00:04Z


+#if !defined(CORRADE_TARGET_EMSCRIPTEN)
+
+struct BackgroundRenderThread {


This is a big struct with lots of code so let's move it to its own file.

eundersander · 2021-05-13T21:03:03Z

+    Mn::GL::Context::makeCurrent(threadContext_);
+    threadOwnsContext_ = true;
+
+    Mn::GL::Renderer::enable(Mn::GL::Renderer::Feature::DepthTest);


Is this bit of render-state logic copy-pasted from Renderer::Impl constructor? I'm worried about maintaining this. Can we avoid the code duplication?

eundersander · 2021-05-13T21:05:56Z

+
+  bool makeCurrentPlatform() {
+    if (!windowlessGLContext_.makeCurrent())
+      Mn::Fatal{} << "Failed to make platform current";


As far as detecting/throwing fatal errors, we also have ESP_CHECK and CORRADE_ASSERT. This style here is new to me. Is it correct?

eundersander · 2021-05-17T17:49:34Z

Have we tested this on Mac?

into ao-migration-async-render

erikwijmans · 2021-06-02T22:41:56Z

@eundersander @aclegg3 @bigbike I refactored this and tried to make the API a little more user friendly, i.e. force people to set physics immediately after starting the async draw.

I also made initialization of the background render thread lazy, so that if people never make async render calls, it will never be initialized and everything will work exactly like before.

aclegg3 · 2021-06-23T17:09:58Z

@erikwijmans Thanks for the work on this. I think it can land if we add a short python smoke test / tutorial script which can be checked on CI by test_examples.py and show users how to use this feature.

into ao-migration-async-render

…okresearch/habitat-sim into ao-migration-async-render

aclegg3 · 2021-06-24T16:31:33Z

@erikwijmans Thanks for adding the example! Agreed with @eundersander that some docstrings for new functions and python bindings would be ideal, but I think this is ready.

eundersander · 2021-06-25T06:00:22Z

@erikwijmans thanks for adding async_rendering.py. I tried running this on my Mac, Fedora, and Ubuntu machines just out of curiosity. It ran fine on Mac but crashed on both Fedora and Ubuntu. Both machines have Nvidia GPUs. Segfault in glDrawElements in the background renderer for apartment_1.glb. Any ideas?

I'm surprised this passed our CI (which is also Ubuntu, I think). How confident are we that this python actually ran as part of https://app.circleci.com/pipelines/github/facebookresearch/habitat-sim/6961/workflows/26578358-7348-4169-ad02-d840053f266c/jobs/34364 ? Is it possible the CI machine ran it with a single hardware thread or otherwise somehow avoided the crash I hit?

eundersander

LGTM

Changes from AO branch supporting async rendering

966e522

aclegg3 requested review from bigbike, erikwijmans and jturner65 May 11, 2021 00:36

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 11, 2021

aclegg3 requested a review from eundersander May 11, 2021 00:38

eundersander reviewed May 13, 2021

View reviewed changes

erikwijmans added 5 commits June 2, 2021 11:36

Merge branch 'master' of https://github.com/facebookresearch/habitat-sim

90184cd

into ao-migration-async-render

Refactor and update the API to be safer

b10d9d8

Merge branch 'master' of https://github.com/facebookresearch/habitat-sim

dca6787

into ao-migration-async-render

Get GL context for BB draw

163df10

Make close destroy by default

8e710d3

erikwijmans requested a review from eundersander June 2, 2021 22:42

erikwijmans added 5 commits June 23, 2021 15:29

Merge branch 'master' of https://github.com/facebookresearch/habitat-sim

73fb07a

into ao-migration-async-render

Add example

615839e

Add example

d87efcb

Merge branch 'ao-migration-async-render' of https://github.com/facebo…

ff4b4f1

…okresearch/habitat-sim into ao-migration-async-render

Fix clang tidy

760c9bb

erikwijmans and others added 2 commits June 25, 2021 14:03

Add comment about headed Linux

34920e8

add docstrings to some new python bindings

fe4e32f

eundersander approved these changes Jun 25, 2021

View reviewed changes

aclegg3 merged commit 81043c5 into master Jun 25, 2021

aclegg3 deleted the ao-migration-async-render branch June 25, 2021 22:20


		#if !defined(CORRADE_TARGET_EMSCRIPTEN)

		struct BackgroundRenderThread {

Conversation

aclegg3 commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

Uh oh!

aclegg3 commented May 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eundersander left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eundersander commented May 17, 2021

Uh oh!

erikwijmans commented Jun 2, 2021

Uh oh!

aclegg3 commented Jun 23, 2021

Uh oh!

aclegg3 commented Jun 24, 2021

Uh oh!

eundersander commented Jun 25, 2021

Uh oh!

eundersander left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aclegg3 commented May 11, 2021 •

edited

Loading