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.

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

aclegg3
Copy link
Contributor

@aclegg3 aclegg3 commented Jan 26, 2023

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)
  • 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.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 26, 2023
@aclegg3
Copy link
Contributor Author

aclegg3 commented Jan 26, 2023

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

Comment on lines +120 to +124
self.display_font = text.FontManager().load_and_instantiate("TrueTypeFont")
self.display_font.open_file(
os.path.join(os.path.dirname(__file__), self.sim_settings["font_path"]),
13,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mosra had to replace the relative filepath with a configurable one which can be updated in consumer code. Generally we can't expect a data folder to be available from conda build, so we should consider how to handle this.

Maybe we should include the font file with the python module source code instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should include the font file with the python module source code instead?

I think that this would make sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, that makes sense -- when John did that originally we weren't sure what the use case would be, and hardcoding the relative path to data/ was the most straightforward way.

@jaraujo98
Copy link
Collaborator

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.

@aclegg3
Copy link
Contributor Author

aclegg3 commented Jan 27, 2023

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. 👍

@mosra
Copy link
Collaborator

mosra commented Feb 2, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants