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

Switch to a multiplatform mmap implementation. #106

Merged
merged 2 commits into from
Jul 19, 2019
Merged

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Jul 15, 2019

Motivation and Context

Should help with #84 -- we don't need to reinvent things that are already implemented and tested :) Also made the code a bit more const-correct and simplified some pointer chasings.

How Has This Been Tested

It builds for me on Linux.

⚠️ Even though the change is pretty straightforward, there's not much to screw up and the underlying API has been extensively tested in other contexts, unfortunately I'm afraid I don't have any PTex mesh data to give this a real test, sorry. Successfully tested with Replica.

Types of changes

  • Docs change / refactoring / dependency upgrade

@mosra mosra requested review from msavva and erikwijmans July 15, 2019 19:55
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jul 15, 2019
@mosra mosra mentioned this pull request Jul 15, 2019
10 tasks
@dhruvbatra
Copy link
Contributor

unfortunately I'm afraid I don't have any PTex mesh data to give this a real test, sorry.

I believe you can find them here:
https://github.com/facebookresearch/Replica-Dataset

@mosra
Copy link
Collaborator Author

mosra commented Jul 15, 2019

Ah! :) Will check those out tomorrow, then.

It has a RAII interface, so no need for explicit unmapping there. Also
made the code more const.
It ensures that CORRADE_TARGET_WINDOWS is always defined on Windows.
Additionally, I switched to CORRADE_TARGET_UNIX from __linux__ (after
excluding CORRADE_TARGET_APPLE) because there's BSD too.
@mosra
Copy link
Collaborator Author

mosra commented Jul 16, 2019

Ok, it took ages to download, but everything seems to work the same way as before! :)

@dhruvbatra
Copy link
Contributor

Great, thanks!

Copy link
Collaborator

@msavva msavva left a comment

Choose a reason for hiding this comment

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

This is great, thanks for cleaning this up @mosra LGTM for landing! 👍

@mosra mosra merged commit 78cebf5 into master Jul 19, 2019
@mosra mosra deleted the mmap-multiplatform branch July 19, 2019 06:52
eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020
…rch#106)

The habitat-api-demo.ipynb notebook illustrates how to specify config parameters, print semantic annotations of a scene. It also visualises the RGB, semantic and depth observations obtained by executing a random sequence of actions.
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
* assets: use a multiplatform Corrade::Utility::Directory::mapRead().

It has a RAII interface, so no need for explicit unmapping there. Also
made the code more const.

* gfx: use Corrade's platform defines for reliable documented behavior.

It ensures that CORRADE_TARGET_WINDOWS is always defined on Windows.
Additionally, I switched to CORRADE_TARGET_UNIX from __linux__ (after
excluding CORRADE_TARGET_APPLE) because there's BSD too.
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.

5 participants