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.

Sign up for GitHub

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

Jump to bottom

Resource manager docs #382

Merged
merged 4 commits into from
Jan 3, 2020
Merged

Resource manager docs #382

merged 4 commits into from
Jan 3, 2020

Conversation

aclegg3
Copy link
Contributor

@aclegg3 aclegg3 commented Dec 16, 2019

Motivation and Context

Documentation push for the ResourceManager and related files/classes. Some related refactoring and cleanup included.

Feedback, proofing, etc... welcome. 👍

How Has This Been Tested

Local build.

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 aclegg3 self-assigned this Dec 16, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 16, 2019
@aclegg3
Copy link
Contributor Author

aclegg3 commented Dec 16, 2019

CI fails including cmake generated header esp/gfx/configure.h for doc linking in ResourceManager.h. Works on local build. Happy for suggestions if anyone knows a simple fix. Otherwise it's not that important and can be removed.

@erikwijmans
Copy link
Contributor

Looks like esp/gfx/configure.h didn't get added to the diff :)

@aclegg3
Copy link
Contributor Author

aclegg3 commented Dec 17, 2019

Looks like esp/gfx/configure.h didn't get added to the diff :)

I think this is actually built from esp/gfx/configure.h.cmake into build/esp/gfx/configure.h. I don't have that file locally in the src directory either. However, my local build works and the CI build failed. 🤔

mosra
mosra reviewed Dec 17, 2019
View reviewed changes
Copy link
Collaborator

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! 🎉

src/esp/assets/BaseMesh.h Show resolved Hide resolved
src/esp/assets/BaseMesh.h Outdated Show resolved Hide resolved
src/esp/assets/BaseMesh.h Show resolved Hide resolved
src/esp/assets/BaseMesh.h Show resolved Hide resolved
src/esp/assets/CollisionMeshData.h Outdated Show resolved Hide resolved
@erikwijmans
Copy link
Contributor

This is why it's failing: https://github.com/facebookresearch/habitat-sim/blob/master/src/esp/gfx/CMakeLists.txt#L66

The configure file needs to be run always.

@aclegg3
Copy link
Contributor Author

aclegg3 commented Dec 17, 2019

This is why it's failing: https://github.com/facebookresearch/habitat-sim/blob/master/src/esp/gfx/CMakeLists.txt#L66

The configure file needs to be run always.

That looks like the culprit, thanks for finding this. I'll update it. 👍

@aclegg3
Copy link
Contributor Author

aclegg3 commented Jan 3, 2020

Any other thoughts on this before merge?

erikwijmans
erikwijmans approved these changes Jan 3, 2020
View reviewed changes
Copy link
Contributor

@erikwijmans erikwijmans left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for doing this @aclegg3

@aclegg3 aclegg3 merged commit 18d0a89 into master Jan 3, 2020
@aclegg3 aclegg3 deleted the resource-manager-docs branch January 3, 2020 21:44
eundersander pushed a commit to eundersander/habitat-sim that referenced this pull request Aug 6, 2020
@dhruvbatra
Update README.md (facebookresearch#382)
01696bb 
Added pointer to documentation.
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
@aclegg3
Resource manager docs (facebookresearch#382)
4e3fd86 
* Documentation pass for ResourceManager.h and related intersecting code. Some related code cleanup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosra mosra mosra left review comments

@erikwijmans erikwijmans erikwijmans approved these changes

@msavva msavva Awaiting requested review from msavva

@mathfac mathfac Awaiting requested review from mathfac

@bigbike bigbike Awaiting requested review from bigbike

@msbaines msbaines Awaiting requested review from msbaines

Assignees

@aclegg3 aclegg3

Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Milestone
Documentation and tutorials improvements
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aclegg3 @erikwijmans @mosra @facebook-github-bot