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

[esp/bindings_js] add initial support for object nav #307

Merged
merged 4 commits into from
Oct 16, 2019
Merged

Conversation

msbaines
Copy link
Contributor

Object and best-view IoU is specified by a URL parameter. If the
user presses the spacebar and IoU of object exceeds 50% of best-view
the task is successfully completed.

Motivation and Context

Required for object nav.

How Has This Been Tested

Verified that object nav is working.

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.

Object and best-view IoU is specified by a URL parameter. If the
user presses the spacebar and IoU of object exceeds 50% of best-view
the task is successfully completed.
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 16, 2019
Copy link
Contributor

@apsdehal apsdehal left a comment

Choose a reason for hiding this comment

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

Some comments. Otherwise, LGTM.


#scope {
position: absolute;
border-color: red;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do
border: 5px dashed red;

opacity: 0.4;
top: 50%;
left: 50%;
margin-left: -160px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really a fan of absolute values as they don't scale according to screen sizes. But, we can focus on it later.

@@ -30,6 +30,7 @@ Module.preRun.push(() => {
let config = {};
config.scene = defaultScene;
buildConfigFromURLParameters(config);
window.config = config;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's build it in your ObjectSensor again. Polluting the global window object is usually not preferred.

@@ -3,6 +3,9 @@
// LICENSE file in the root directory of this source tree.

/*global Module */
import ObjectSensor from "./object_sensor";

const IOU_TOLERANCE = 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to defaults?

Comment on lines 37 to +58
this.semanticObservation = new Module.Observation();
this.semanticObjects = this.sim.sim.getSemanticScene().objects;
this.semanticScene = this.sim.sim.getSemanticScene();
this.semanticObjects = this.semanticScene.objects;

if (window.config.category) {
const scopeWidth = this.components.scope.offsetWidth;
const scopeHeight = this.components.scope.offsetHeight;
const scopeInsetX = (this.components.canvas.width - scopeWidth) / 2;
const scopeInsetY = (this.components.canvas.height - scopeHeight) / 2;
const objectSearchRect = {
left: scopeInsetX,
top: scopeInsetY,
right: scopeInsetX + scopeWidth,
bottom: scopeInsetY + scopeHeight
};
this.objectSensor = new ObjectSensor(
objectSearchRect,
this.semanticShape,
this.semanticScene,
window.config.category
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a good time to move this to its own function?

}
}

console.log(this.objectIds);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

}
}
for (let i = 0; i < objects.size(); i++) {
const object = objects.get(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const object = objects.get(i);
const object = objects.get(i) || {};

for (let i = 0; i < objects.size(); i++) {
const object = objects.get(i);
if (
object &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
object &&

* @param {String} className - object class to search for
*/
computeIOU(observation) {
const width = this.semanticShape.get(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to utils.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might not make sense to move to utils with objectIds check below.

}
}
}
if (maskPixelsIn === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this will be automatically captured in statement below. For speed purposes, checking this in the start would be better without even iterating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value is computed in the loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant this check is not needed as without this check it will still return zero.

I thought you were saving on rectArea calculation. If that was the case, this can be precomputed by checking top, left, bottom, right of both triangles beforehand.

@msbaines msbaines merged commit 40098e4 into master Oct 16, 2019
@msbaines msbaines deleted the scope branch October 16, 2019 22:39
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
…h#307)

Object and best-view IoU is specified by a URL parameter. If the
user presses the spacebar and IoU of object exceeds 50% of best-view
the task is successfully completed.
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

3 participants