Skip to content

ManagedObjects : CSV Informational Reports#1331

Merged
jturner65 merged 25 commits intofacebookresearch:masterfrom
jturner65:MM_InfoReports
Jun 22, 2021
Merged

ManagedObjects : CSV Informational Reports#1331
jturner65 merged 25 commits intofacebookresearch:masterfrom
jturner65:MM_InfoReports

Conversation

@jturner65
Copy link
Copy Markdown
Contributor

@jturner65 jturner65 commented Jun 21, 2021

Motivation and Context

This PR introduces reporting functionality for loaded attributes and wrappers from their respective managers, and the Metadata Mediator. Functions are provided that will construct csv-friendly strings of data from each attributes and managed object, and lists of these strings for each manager. The MetadaMediator can be queried to provide an aggregate report of all loaded configurations.

This functionality is of particular importance with upcoming MetadataMediator/SceneDataset/SceneInstance Documentation and Tutorials. This also has a simple python writer function provided.

How Has This Been Tested

C++ and python tests pass. The MetadataMediator test has been expanded to test this functionality, although currently it just displays a report.

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.

@jturner65 jturner65 requested review from Skylion007 and aclegg3 June 21, 2021 20:13
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jun 21, 2021
// Must always be valid value
ObjectInstanceShaderType shaderType =
static_cast<ObjectInstanceShaderType>(translationOrigin);
for (const auto& it : ShaderTypeNamesMap) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We really may want to invest in a bimap implementation

Comment thread src/esp/metadata/attributes/LightLayoutAttributes.cpp Outdated
Comment thread src/esp/metadata/attributes/SceneAttributes.cpp Outdated
@Skylion007
Copy link
Copy Markdown
Contributor

The biggest issue here is that the string are not built efficiently. See https://clang.llvm.org/extra/clang-tidy/checks/performance-inefficient-string-concatenation.html

Copy link
Copy Markdown
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

@jturner65 jturner65 requested a review from Skylion007 June 22, 2021 13:19
Comment thread src/esp/metadata/MetadataMediator.cpp
.append(lightInst.second->getObjectInfoHeader())
.append("\n");
}
res.append(", ").append(lightInst.second->getObjectInfo()).append("\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again here (pretty sure there is a more efficient overload for appending chars.

.append(",\n");

// stage instance info
res.append(stageInstance_->getObjectInfo()).append("\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

char

// stages
std::vector<std::string> stageAttrInfoAra =
stageAttributesManager_->getObjectInfoStrings();
for (const std::string& s : stageAttrInfoAra) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here again

Copy link
Copy Markdown
Contributor

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

Chars overloads

Comment thread src/esp/bindings/PhysicsWrapperManagerBindings.cpp Outdated
Comment thread src/esp/metadata/MetadataMediator.cpp Outdated
for (const auto& lightInst : lightInstances_) {
if (iter == 0) {
iter++;
res.append(1, ',')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

BTW res+= for every item is also viable here.

@jturner65
Copy link
Copy Markdown
Contributor Author

Here's an example report generated on the AI2Thor scene instances I generated a few months ago, to give an idea of what this will look like.
ai2thor_first_report.csv

@jturner65 jturner65 merged commit d2ccdcb into facebookresearch:master Jun 22, 2021
@jturner65 jturner65 deleted the MM_InfoReports branch June 22, 2021 23:17
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.

3 participants