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

Support instance sensor (it can be used in viewer.cpp directly) #2080

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Jianghanxiao
Copy link

@Jianghanxiao Jianghanxiao commented Apr 21, 2023

Motivation and Context

The instance sensor can directly give the instance id for each object, like the semantic sensor for semantic id. But instance sensor is much more flexible. Given a instance id, we can fetch anything we want, including the semantic id. And in this way, we can easily change the semantic mapping without creating a new dataset, but just manually give some new mapping.

How Has This Been Tested

This sensor has been tested in the viewer.cpp. Now if you use key '7' to switch the sensor, the fourth sensor is the instance sensor after the semantic sensor.

Brief Explanation of the implementation

To implement the instance sensor, I follow the logic of semantic sensor. For the semantic sensor, it will fetch the object semantic id and bake it into the buffer of GL. I add some logical condition to judge if it's an instance sensor, then fetch the instance id. (I additionally store the instance id into the object when creating the scene using the objects). The modification is very simple and easy to use. (No python wrapper for now). P.S. For the instance sensor, the reason why some objects have similar color is that their instance id are adjancent. That's why they have similar color. (e.g. the chairs around the table).

Screenshare.-.2023-04-14.3_24_25.PM.mp4

If I manually times the instance id with 3, the color will be much different
image (2)

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 Apr 21, 2023
@Jianghanxiao Jianghanxiao changed the title support instance sensor -> it can be used in viewer.cpp directly Support instance sensor (it can be used in viewer.cpp directly) Apr 21, 2023
@Jianghanxiao
Copy link
Author

@aclegg3 Can you give some suggestions on this?

src/CMakeLists.txt Outdated Show resolved Hide resolved
@jturner65
Copy link
Contributor

This is really great. Can you please add appropriate documentation describing the purpose and meaning of instanceID in relation to how it is being used?

@Jianghanxiao
Copy link
Author

Jianghanxiao commented Apr 27, 2023

This is really great. Can you please add appropriate documentation describing the purpose and meaning of instanceID in relation to how it is being used?

Thanks!! Sure, I will add some documentation to further describe the purpose and the meaning of instance ID. Actually this is my first time contributing to habitat-sim, for documentation, I just need to add some comments for the new functions in the documentation style? Or I should do some other things to help better explain it?

@Jianghanxiao
Copy link
Author

I add some more comments for the defined functions with the documentation style. But I'm not sure where is the best place to add the purpose and meaning of instance ID. I will post it here for now.

Purpose: instance ID is easy to fetch objects in each scene. And in the meanwhile, we can attach/modify/fetch attributes for a specific object much easier. In addition, instance ID is a natural idea for the object-level representation in each scene. Therefore, to add an instance ID, we can directly use the implicit index of the object in each scene as the instance ID, and add them into the object attributes when loading the scene using the preloaded objects code

Meaning: the instance ID for each object is unique in each scene. It can be the implicit index of the objects defined in the scene configs.

How to use: currently I add a new camera type called "Instance", when using this camera, the rendering results will directly give back the instance ID of the objects like the semantic sensor. (For the implementation, it's very simple, just add a flag in the camera to tell if it should fetch the semantic ID or the instance ID. Then the draw function will fetch the flag to see if it should use semantic id or instance id)

@jturner65
Copy link
Contributor

jturner65 commented Apr 27, 2023

Thanks!! Sure, I will add some documentation to further describe the purpose and the meaning of instance ID. Actually this is my first time contributing to habitat-sim, for documentation, I just need to add some comments for the new functions in the documentation style? Or I should do some other things to help better explain it?

Well, congrats on a great first PR! The comments that you added look good, and your description you shared above is good too. Even just having it in the PR will at least provide folks with some context.

@aclegg3 Is there a docs on aiHabitat that this info could be added to?

@Jianghanxiao
Copy link
Author

Jianghanxiao commented Apr 28, 2023

@jturner65 Hi, I just found that there are some updates in the main branch about the materials or something. I try merging the update, however, I find that after merging, the semantic sensor and instance sensor don't work. So I try directly building the latest main branch without any instance sensor stuff. The semantic sensor still doesn't work. I think there may be some bug in current latest update? Should I create an issue for it? I will not merge the update before the semantic sensor works in the main branch. After my testing, the bug is introduced after the #2083

@jturner65
Copy link
Contributor

@jturner65 Hi, I just found that there are some updates in the main branch about the materials or something. I try merging the update, however, I find that after merging, the semantic sensor and instance sensor don't work. So I try directly building the latest main branch without any instance sensor stuff. The semantic sensor still doesn't work. I think there may be some bug in current latest update? Should I create an issue for it? I will not merge the update before the semantic sensor works in the main branch. After my testing, the bug is introduced after the #2083

Please make an issue regarding this in the tracker, being sure to include sufficiently detailed steps for reproducing it. FWIW, the semantic sensor works fine for me on main.

@jturner65
Copy link
Contributor

semantics

@Jianghanxiao
Copy link
Author

Jianghanxiao commented Apr 29, 2023

@jturner65 Hi, I just found that there are some updates in the main branch about the materials or something. I try merging the update, however, I find that after merging, the semantic sensor and instance sensor don't work. So I try directly building the latest main branch without any instance sensor stuff. The semantic sensor still doesn't work. I think there may be some bug in current latest update? Should I create an issue for it? I will not merge the update before the semantic sensor works in the main branch. After my testing, the bug is introduced after the #2083

Please make an issue regarding this in the tracker, being sure to include sufficiently detailed steps for reproducing it. FWIW, the semantic sensor works fine for me on main.

Emmm, really? Let me try building again. Currently based on my test, at least, it doesn't work in FP dataset and HM3D dataset. This is the command I use to build python setup.py install --with-cuda --bullet --build-type Debug.

This is what I observe for the semantic sensor using viewer.cpp
image

I further try locating the problem. I find that the difference is here. Line of code. This one will be True when loading the FP dataset, and it always returns 0. The exact bug is in issue #2087

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.

4 participants