habitat-sim icon indicating copy to clipboard operation
habitat-sim copied to clipboard

[WIP] - Add viewer.py to Habitat-sim module

Open aclegg3 opened this issue 2 years ago • 4 comments

Motivation and Context

Currently, the interactive sim viewer (example/viewer.py) lives in the examples/ directory is therefore not exposed within the habitat_sim python module. As such, it cannot be used with Habitat-sim conda build or imported and extended in Habitat-lab.

This PR copies the viewer code into the src_python directory and makes minor edits for code style and flaking requirements.

See Habitat-lab PR 1099 for example use as external module to extend.

TODO: Once this is ready for merge we'll update documentation and possibly deprecate the examples/viewer.py.

How Has This Been Tested

With Habitat-sim module installed,

  1. run the new viewer from its python file locally
python src_python/habitat_sim/viewer/viewer.py --dataset data/scene_datasets/mp3d_example/mp3d.scene_dataset_config.json --scene 17DRP5sb8fy
  1. run the new viewer from the module:
python -m habitat_sim.viewer.viewer --dataset data/scene_datasets/mp3d_example/mp3d.scene_dataset_config.json --scene 17DRP5sb8fy
  • Issue: module run is currently failing with:
Platform::WindowlessEglContext: cannot make context current: EGL_BAD_ACCESS
Platform::WindowlessEglApplication::makeCurrent(): cannot make context current: EGL_BAD_ACCESS
Failed to make platform current

Likely related: we require the following to avoid this issue with the python file run:

flags = sys.getdlopenflags()
sys.setdlopenflags(flags | ctypes.RTLD_GLOBAL)

possibly the module run is picking things up differently.

Types of changes

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

Checklist

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

aclegg3 avatar Jan 26 '23 20:01 aclegg3

@mosra thoughts on the EGL issue with running from the module?

aclegg3 avatar Jan 26 '23 20:01 aclegg3

I'm a strong supporter of this change! A couple of thoughts (from when I was thinking about this a few months ago):

  • Several of the current keybindings make sense for an example viewer, but might not be interesting for every viewer application, and it is helpful to have those keys free for different functionality. From that perspective, I think it could make sense to keep keybindings to a minimum in the base viewer class and add the remaining ones in an ExampleViewer subclass implemented in the examples/viewer.py file.
  • The same thing goes for the command line arguments. This makes sense for the example viewer, but maybe not for a base class definition inside src_python.

jaraujo98 avatar Jan 27 '23 08:01 jaraujo98

keep keybindings to a minimum in the base viewer class and add the remaining ones in an ExampleViewer subclass implemented in the examples/viewer.py file... The same thing goes for the command line arguments...

Thanks, I agree. I'll start a design doc to help plan which UI features should be core vs. example. :+1:

aclegg3 avatar Jan 27 '23 18:01 aclegg3

sys.setdlopenflags(flags | ctypes.RTLD_GLOBAL)

A bit of background, just to have the full picture here -- by default, Python loads every module in its own namespace (RTLD_LOCAL), making it impossible for a different module to see its globally exported symbols. This makes sense for security reasons. RTLD_GLOBAL disables that. I'm not sure why the behavior differs between python -m module and python module.py, but it's probably documented somewhere.

The other half of the problem that Habitat uses Magnum compiled statically, and while I eliminated most mutable globals, Magnum has still three left -- for the plugin manager, for logging redirection and for the GL context. Those, with Magnum being compiled statically, get one copy in the habitat_sim.so Python module and one in the magnum.so Python module, and then I have to do cursed things to make both modules refer to the same globals so operation done in one module is visible by the other. And those cursed things then only work with RTLD_GLOBAL. Without that, being each loaded in its own namespace, the modules would need to share a common address of the globals in some manual way, possibly by going through Python, and so far I didn't figure out a nice way to do this that wouldn't become a new problem on its own.

A real solution to all these problems would be to have Magnum compiled dynamically, but that was historically not an option as far as I remember. Maybe we could reconsider, it'd definitely fix 98 of 100 recurring nightmares I have :D

The EGL error is most probably related to the above -- EGL_BAD_MATCH is emitted if the context is already active in another thread, so it's possible that due to the global GL context not being shared properly between the two modules it was thinking the context wasn't active in another thread (while it actually was, for example in the background renderer if that's enabled in the build) and tried to use it.


TL;DR: you'll still need the flag there, at least until Magnum gets compiled dynamically, or a different solution is discovered.

mosra avatar Feb 02 '23 19:02 mosra