Cleanup and development of flag to enable/disable rendering by ldcWV · Pull Request #1308 · facebookresearch/habitat-sim

ldcWV · 2021-06-10T22:11:02Z

Motivation and Context

This PR is a continuation of Eric's PR here: #1258

Here is the todo list in the PR and how I addressed them:

The hacky use of a global g_createMagnumRenderer var.

The flag is now stored in SimulatorConfiguration and passed around using getter methods. GenericDrawable also has a copy of the flag that is set in its constructor.

Probably not all cases of graphics context use have been properly disabled/guarded, e.g. PbrDrawable.

Turns out the PbrDrawable codepath is impossible to reach when createRenderer=false. See #1308 (comment)

Let's add a test of creating a non-renderer sim and using various APIs like drawObservation.

See SimTest.cpp.

We should handle primitives correctly or at least fail gracefully. If we need to disable primitive-loading/use, we should issue a proper warning to users if they try to use a primitive that's unavailable.

This was referring to the skipping of the ResourceManager::initDefaultPrimAttributes method if the flag is off. The concern was that because of this, primitives would not be loaded correctly and thus should not be used. However, it appears that the only thing disabling this method affects is the visualization of bounding boxes, which doesn't matter when not using a renderer anyways.

We need to reconcile with the existing SimulatorConfiguration::createRenderer. From discussion with @jturner65 , it sounds like the existing functionality of the existing flag (including disabling physics manager) isn't needed at all. So we instead hook up the createRenderer flag to the new functionality introduced in this PR (and discard the no-physics-manager code path altogether?).

The old createRenderer flag and its codepath have been deleted. The new flag has been renamed to createRenderer.

Decide whether to create a stub Renderer object (discussed below).

Decided in meeting against this approach.

reconcile two related features: disabling usage of OpenGL (this PR) and disabling loading of render assets (separate feature). After team discussion on 6/1, we agree this can be implemented separately. However, one suggestion is to have an enum for all the combinations of usage, instead of two independent flags. However, we all agreed that this PR doesn't need to implement the second feature, disabling loading of render assets (that can be a future PR).

Done (first feature). GL::Mesh and Shaders::FlatGL are no longer created when the flag is set.

How Has This Been Tested

A test case demonstrating the functionality (what still works, what doesn't work) when the flag is disabled has been added in SimTest.cpp.

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.

…mulator without a Magnum/OpenGL context

…te_magnum_renderer=False

into eundersander/new_create_renderer_flag

…Test.cpp

…createDrawable to constructors of GenericDrawable and PbrDrawable

…ateRenderer

…tive_meshes_

eundersander

Good progress here. I left some comments.

eundersander · 2021-06-16T17:42:46Z


  scene::SceneNode& node_;
-  Magnum::GL::Mesh& mesh_;
+  Magnum::GL::Mesh* mesh_ = nullptr;


Can you post a reply here and explain to other reviewers why we did this change?

The createRenderer flag prevents the function GenericMeshData::uploadBuffersToGPU from running, which is the function that creates GL::Mesh. This means that the GenericMeshData that are inserted into ResourceManager::meshes_ will not have set GL::Mesh fields.

However, getMagnumGLMesh is still being called on elements of meshes_. These calls will return NULL because no GL::Mesh objects were ever created. Yet we are still trying to do things with the returned values, which is unsafe.

Therefore, we instead store pointers to the GL::Mesh objects so that we must explicitly dereference them, which requires first checking that they are not nullptr. This allows us to avoid undefined behavior by failing fast.

Skylion007

@bigbike Should be the one to look at this considering how it intertwines with the GFX system.

mosra · 2021-06-23T14:25:32Z

FYI the GL::Mesh can be constructed as GL::Mesh{NoCreate} without a GL context present. So you could keep passing GL::Mesh& around, which could reduce the amount of changes and additional logic in this PR quite a bit.

Same is for textures and other GL objects (GL::Texture2D{NoCreate} won't need a GL context to exist), I'll check if the Bullet DebugDraw could get a similar constructor as well.

(Apologies if this suggestion came too late, was too busy to check PR reviews regularly.)

eundersander

LGTM. Let's get the unit tests passing and get this merged.

bigbike

Please fix all the places that I mentioned, before committing it in.

eundersander · 2021-06-23T17:20:31Z

FYI the GL::Mesh can be constructed as GL::Mesh{NoCreate} without a GL context present.

As you say, this suggestion is coming a bit late. :) For this PR, we removed GL::Mesh creation as you originally requested in our Slack discussions. This PR has been under review for a while and we'd like to get it merged. Let's do the switch to GL::Mesh{NoCreate} as a separate refactor PR.

bigbike · 2021-06-23T17:22:20Z

FYI the GL::Mesh can be constructed as GL::Mesh{NoCreate} without a GL context present.

@mosra : Great. Good to know. However, I guess one reason they changed them to pointer is that they can easily tell if the simulator is initialized with the renderer (just checking if the pointer is NULL).
In the way you suggested, how can this be done?

ldcWV · 2021-06-23T18:47:16Z


 void GenericDrawable::draw(const Mn::Matrix4& transformationMatrix,
                           Mn::SceneGraph::Camera3D& camera) {
-  if (!isRendererCreated) {


It turns out that this block is not necessary because the GenericDrawable::draw codepath is impossible to reach when createRenderer==false.

bigbike · 2021-06-24T17:14:33Z

+    cfg.createRenderer = false;
    metadataMediator_ = MetadataMediator::create(cfg);
    resourceManager_ = std::make_unique<ResourceManager>(metadataMediator_);
-    context_ = esp::gfx::WindowlessContext::create_unique(0);


Wait, if you change this, then we lose the protection from the unit test when the context needs to be created.

In your case, you need to create a brand new test function to test when the Renderer is not created.

Doing this shows that you don't need a context in order to do physics when createRenderer==false, which is good to know. Maybe I can modify the tests to try both with and without context. Do you think this is a good idea?

I already added a non-physics test with createRenderer=false in SimTest.cpp by the way.

Sorry for late reply here. I want to see this change stay as-is.

I actually love that we're now guaranteed not to use OpenGL in our Physics unit tests. Conceptually, physics tests shouldn't rely on rendering. Let's imagine a future day where we've completely decoupled physics and rendering code--it'll be nice that these tests just port over directly, without having to be fixed up. By never creating a context here, we're guaranteed that our physics test code is "clean"--not accidentally relying on some rendering feature.

we lose the protection from the unit test when the context needs to be created.

If you have in mind a unit test that involves physics and rendering, the best place for that is SimTest. Simulator is where we integrate physics and rendering. You can add objects, make them interact with physics, then call draw() or whatever and check the result.

This has nothing to do with the visualization verification. We do that in the SimTest. No problem.

My opinion is that by doing both (with or without gl context) in the physicsTest, we have solid proof that physics simulation and the rendering are totally orthogonal to each other. Both tests should pass.

Do we have a test in the SimTest that creates a GL context and enabled and timestepped physics? No, we do not have.
We used to count on this physicsTest here until this change.

My opinion is that by doing both (with or without gl context) in the physicsTest, we have solid proof that physics simulation and the rendering are totally orthogonal to each other. Both tests should pass.

Ok, fair enough. We'll try to find a way to run both versions of the physics tests, hopefully without duplicating a lot of code.

Yeah, I hear you.

Too bad we are using the google test, not the corrade test (used in the SimTest).
Corrade test supports instanced tests. See here.

This diff is to add a new feature which can enable/disable the rendering. I saw Lawrence added tests in the SimTest. So I assume this feature is well covered.

We also know the current solution is a temporary solution as we will refactor this code in the near future. So I am totally fine if we do not touch the PhysicsTest.cpp at all in this PR. I think it is the easiest way to unblock it.

Are you comfortable with this solution?

I've made a commit to run PhysicsTest both with and without the flag set.

…possible settings of createRenderer

into ldchen/new_create_renderer_flag

aclegg3

LGTM.

eundersander and others added 9 commits May 21, 2021 00:39

add SimulatorConfiguration::createMagnumRenderer and support for a Si…

093eb61

…mulator without a Magnum/OpenGL context

modify PhysicsTest and GfxReplayTest to not use a graphics context

43d76ed

python binding for SimulatorConfiguration.createMagnumRenderer

6cdf145

update python to handle case of null _sim.renderer

39c5209

add make_video_during_simulation and test SimulatorConfiguration.crea…

0a8afc5

…te_magnum_renderer=False

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

f98a10c

into eundersander/new_create_renderer_flag

Use getters to access createMagnumRenderer flag; create a test in Sim…

15a45db

…Test.cpp

Better develop tests

3549a46

Add todo

f2b7040

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

ldcWV marked this pull request as draft June 10, 2021 22:12

Fix formatting

a6e07ff

ldcWV requested review from Skylion007, bigbike, eundersander and jturner65 June 10, 2021 22:15

Lawrence Chen and others added 6 commits June 15, 2021 11:23

Add additional block in loadNavMeshVisualization and move block from …

49b9974

…createDrawable to constructors of GenericDrawable and PbrDrawable

Remove old createRenderer flag and rename createMagnumRenderer to cre…

1f40106

…ateRenderer

Fix formatting

3882f0f

Add block in loadNavMeshVisualization before it tries to add to primi…

0bba6bf

…tive_meshes_

change Drawable to hold a pointer to mesh instead of ref

1515804

Disable shaders when flag is set by adding a block at updateShaders()

51e9fbf

ldcWV marked this pull request as ready for review June 16, 2021 00:04

ldcWV requested a review from aclegg3 June 16, 2021 00:04

eundersander requested a review from mosra June 16, 2021 17:30

eundersander reviewed Jun 16, 2021

View reviewed changes

Comment thread src/tests/SimTest.cpp Outdated

Minor refactoring and documentation

89ca164

Skylion007 reviewed Jun 16, 2021

View reviewed changes

Comment thread examples/tutorials/nb_python/replay_tutorial.py Outdated

Skylion007 reviewed Jun 16, 2021

View reviewed changes

Skylion007 approved these changes Jun 23, 2021

View reviewed changes

Skylion007 reviewed Jun 23, 2021

View reviewed changes

Comment thread src/esp/assets/ResourceManager.cpp Outdated

eundersander approved these changes Jun 23, 2021

View reviewed changes

bigbike reviewed Jun 23, 2021

View reviewed changes

Comment thread src/esp/gfx/GenericDrawable.cpp Outdated

Comment thread src/esp/gfx/GenericDrawable.cpp Outdated

Improve fail fast in drawable classes

4bc0f41

ldcWV commented Jun 23, 2021

View reviewed changes

eundersander mentioned this pull request Jun 23, 2021

New flag to enable a Simulator that doesn't create a Magnum/OpenGL context #1258

Closed

11 tasks

Merge branch 'master' into ldchen/new_create_renderer_flag

db1af4b

ldcWV requested a review from bigbike June 23, 2021 19:00

bigbike suggested changes Jun 24, 2021

View reviewed changes

bigbike reviewed Jun 24, 2021

View reviewed changes

Comment thread src/esp/assets/ResourceManager.cpp Outdated

bigbike reviewed Jun 24, 2021

View reviewed changes

Comment thread src/esp/assets/ResourceManager.cpp Outdated

bigbike reviewed Jun 24, 2021

View reviewed changes

Comment thread src/esp/gfx/MeshVisualizerDrawable.cpp Outdated

Comment thread src/esp/gfx/PTexMeshDrawable.cpp Outdated

Comment thread src/esp/gfx/PbrDrawable.cpp Outdated

bigbike reviewed Jun 24, 2021

View reviewed changes

Comment thread src/esp/gfx/MeshVisualizerDrawable.cpp Outdated

Lawrence Chen and others added 5 commits June 24, 2021 13:43

Minor style fixes

4f20f96

Minor

5f5023d

Fix error message

db7465d

Add parameterized tests in PhysicsTest.cpp to test physics with both …

ee5c324

…possible settings of createRenderer

Merge branch 'master' into ldchen/new_create_renderer_flag

58007ce

ldcWV requested a review from bigbike June 26, 2021 01:04

Lawrence Chen added 2 commits June 29, 2021 11:44

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

236311d

into ldchen/new_create_renderer_flag

Fix CI test

51416ce

bigbike approved these changes Jul 1, 2021

View reviewed changes

aclegg3 approved these changes Jul 1, 2021

View reviewed changes

ldcWV merged commit d58e53f into facebookresearch:master Jul 1, 2021

erikwijmans mentioned this pull request Jul 12, 2021

Empty RigidBodyManager when there are no sensors #1378

Closed

jturner65 mentioned this pull request Apr 10, 2023

--[BugFix]Testing disabled renderer in simtest should only happen 1 time #2065

Merged

11 tasks

Conversation

ldcWV commented Jun 10, 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!

eundersander left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eundersander Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

ldcWV Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Skylion007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mosra commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eundersander left a comment

Choose a reason for hiding this comment

Uh oh!

bigbike left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eundersander commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bigbike commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldcWV Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bigbike Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

ldcWV Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eundersander Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigbike Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigbike Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldcWV commented Jun 10, 2021 •

edited

Loading

ldcWV Jun 16, 2021 •

edited

Loading

mosra commented Jun 23, 2021 •

edited

Loading

eundersander commented Jun 23, 2021 •

edited

Loading

bigbike commented Jun 23, 2021 •

edited

Loading

ldcWV Jun 24, 2021 •

edited

Loading

eundersander Jun 24, 2021 •

edited

Loading

bigbike Jun 24, 2021 •

edited

Loading

bigbike Jun 24, 2021 •

edited

Loading

eundersander Jun 24, 2021 •

edited

Loading