[Bug Fix] Fixing rotational bug in `tasks.rearrange.utils.get_camera_transform` #1847

zephirefaith · 2024-03-11T16:21:24Z

Motivation and Context

Fixes issue #1804

How Has This Been Tested

Tested with zf/obs-wm-updates branch of habitat-llm. Following is the relevant driver code:

fixed_pose = get_camera_transform(
                    self.sim.agents_mgr._all_agent_data[0].articulated_agent,
                    camera_name=f"{curr_agent}_{camera_source}_rgb",
                )

Check fixed point-clouds generated through this method in this PR comment.

Types of changes

[Docs change] Addition or changes to the documentation
[Refactoring] Large changes to the code that improve its functionality or performance
[Dependency Upgrade] Upgrades one or several dependencies in habitat
[Bug Fix] (non-breaking change which fixes an issue)
[Development] A pull request that add new features to the habitat-lab task and environment codebase. Development Pull Requests must be small (less that 500 lines of code change), have unit testing, very extensive documentation and examples. These are typically new tasks, environments, sensors, etc... The review process for these Pull Request is longer because these changes will be maintained by our core team of developers, so make sure your changes are easy to understand!
[Experiment] A pull request that add new features to the habitat-baselines training codebase. Experiments Pull Requests can be any size, must have smoke/integration tests and be isolated from the rest of the code. Your code additions should not rely on other habitat-baselines code. This is to avoid dependencies between different parts of habitat-baselines. You must also include a README that will document how to use your new feature or trainer. You will be the maintainer of this code, if the code becomes stale or is not supported often enough, we will eventually remove it.

Checklist

My code follows the code style of this project.
I have updated the documentation if required.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes if required.

habitat-lab/habitat/tasks/rearrange/utils.py

jimmytyyang

LGTM! If would be nice if you could add a comment or flag about how this was computed before so that I can make sure my skill training code is correct and I will fix them.

zephirefaith · 2024-03-15T15:07:49Z

Based on offline conversations:

@aclegg3's concern that the current code uses value of (0, 0, 0) as an unassigned default while it is a valid setting. Should be replaced with None instead.
Let's wrap get_camera_from_link logic as a util and use it consistently in setter + getter in: habitat.tasks.rearrange.utils.get_camera_transform, kinematic_humanoid.update and manipulator.update
Add a print warning notifying change in logic and a use_deprecated_logic flag to make sure logic change does not break things for other users who depend on old logic (CC: @jimmytyyang )

…f/get-cam-transform-fix

zephirefaith · 2024-03-19T19:14:39Z

@jimmytyyang @xavierpuigf Please take a look at this change as it directly touches your agent functions.

@aclegg3 Pushed the changes we discussed, please take a look and leave comments. However, let's not merge this until Xavi has taken a look and approved. I already walked Jimmy through this and I think the code changes will support his use-case.

aclegg3

This looks much better, thanks for the fix. 👍

I'll request that you add some typing/docs and I suggest you don't worry about backward compatibility unless there is a known down-stream user to accommodate.
We can merge when @xavierpuigf approves.

As an aside, I think this util function could be unit tested in test_robot_wrapper.

aclegg3 · 2024-03-20T22:29:04Z

habitat-lab/habitat/articulated_agents/utils.py

+        rot_offset = articulated_agent.offset_transform_base.inverted()
+        link_trans = articulated_agent.base_transformation @ rot_offset
+        # else:
+        # raise Exception(


Keeping or leaving this exception logic?

I want to keep it but running into a LOT of cyclic import problems when I try to use typing in this function.

This function is used in manipulator.py however, the arguments of this function are inherited children of Manipulator class. So if I add types, it leads to manipulator.py ending up importing Manipulator in-turn. We could just use Manipulator as the arg-type here but that is very generic and does not let us add guardrails like the exception I commented out. (importing KinematicHumanoid was leading to a cyclic import issue).

Is there some better code organization we could do here?

aclegg3 · 2024-03-20T22:30:03Z

habitat-lab/habitat/articulated_agents/utils.py

+def get_articulated_agent_camera_transform_from_cam_info(
+    articulated_agent,
+    cam_info,
+):
+    # Get the camera's attached link


Please add type hints and a docstring to clarify the parameters and expose to the web API docs

See the problem mentioned above with typing :(

habitat-lab/habitat/articulated_agents/utils.py

aclegg3 · 2024-03-21T00:25:26Z

habitat-lab/habitat/tasks/rearrange/utils.py

-    cam_trans = link_trans @ offset_trans @ cam_info.relative_transform
+    cam_trans = None
+
+    if deprecated_logic:


Interesting strategy. Do we need this? Are there active down-stream use cases we are tracking?

IMO We can change the logic without back-compat and mark this PR as a breaking change since it is so close to the last release.

I agree with this. I think @jimmytyyang is the only known down-stream user who was using this function. I walked through his code with him and I do not think this change should affect him. He was using the pose to do ray-casting from the center of the camera, this function only adds in-plan rotation which should not upset his logic.

sounds good to me! priyam you made a call for this!

Thanks! I'll take this out so we have a cleaner code-merge.

Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com>

xavierpuigf

LGTM, thanks! Agreed that if we don't use the decprecated logic, it may be fine to remove it

…f/get-cam-transform-fix

…b into zf/get-cam-transform-fix" This reverts commit 5c1c555, reversing changes made to 0f2d71f.

cam is offset in trans + rot

e5b474c

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 11, 2024

consider all cases

eadfa1e

zephirefaith marked this pull request as ready for review March 14, 2024 03:00

zephirefaith requested review from jimmytyyang, aclegg3 and SergioArnaud March 14, 2024 03:01

zephirefaith changed the title ~~Fixing rotational bug in tasks.rearrange.utils.get_camera_transform~~ [Bug Fix] Fixing rotational bug in tasks.rearrange.utils.get_camera_transform Mar 14, 2024

pre-commit

76b569d

aclegg3 reviewed Mar 14, 2024

View reviewed changes

habitat-lab/habitat/tasks/rearrange/utils.py Outdated Show resolved Hide resolved

aclegg3 reviewed Mar 14, 2024

View reviewed changes

habitat-lab/habitat/tasks/rearrange/utils.py Outdated Show resolved Hide resolved

jimmytyyang approved these changes Mar 15, 2024

View reviewed changes

zephirefaith added 3 commits March 19, 2024 18:22

Merge branch 'main' of github.com:facebookresearch/habitat-lab into z…

d6ee477

…f/get-cam-transform-fix

change initialization to None

f792707

add getter logic which is used across all use-cases

8895f2d

zephirefaith requested a review from xavierpuigf March 19, 2024 19:14

zephirefaith added 2 commits March 19, 2024 19:36

remove typing and circular imports

6ba4c4f

code reorg to eliminate circular imports

ebac53c

aclegg3 reviewed Mar 21, 2024

View reviewed changes

Update habitat-lab/habitat/articulated_agents/utils.py

0f2d71f

Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com>

xavierpuigf approved these changes Mar 25, 2024

View reviewed changes

An HPCaaS user for priparashar and others added 2 commits April 10, 2024 18:35

Merge branch 'main' of github.com:facebookresearch/habitat-lab into z…

5c1c555

…f/get-cam-transform-fix

Revert "Merge branch 'main' of github.com:facebookresearch/habitat-la…

b35b9b4

…b into zf/get-cam-transform-fix" This reverts commit 5c1c555, reversing changes made to 0f2d71f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug Fix] Fixing rotational bug in `tasks.rearrange.utils.get_camera_transform` #1847

[Bug Fix] Fixing rotational bug in `tasks.rearrange.utils.get_camera_transform` #1847

zephirefaith commented Mar 11, 2024 •

edited

jimmytyyang left a comment

zephirefaith commented Mar 15, 2024

zephirefaith commented Mar 19, 2024 •

edited

aclegg3 left a comment •

edited

aclegg3 Mar 20, 2024

zephirefaith Mar 25, 2024

aclegg3 Mar 20, 2024

zephirefaith Mar 25, 2024

aclegg3 Mar 21, 2024

zephirefaith Mar 25, 2024

jimmytyyang Mar 25, 2024

zephirefaith Mar 25, 2024

xavierpuigf left a comment

[Bug Fix] Fixing rotational bug in tasks.rearrange.utils.get_camera_transform #1847

Are you sure you want to change the base?

[Bug Fix] Fixing rotational bug in tasks.rearrange.utils.get_camera_transform #1847

Conversation

zephirefaith commented Mar 11, 2024 • edited

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

jimmytyyang left a comment

Choose a reason for hiding this comment

zephirefaith commented Mar 15, 2024

zephirefaith commented Mar 19, 2024 • edited

aclegg3 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierpuigf left a comment

Choose a reason for hiding this comment

[Bug Fix] Fixing rotational bug in `tasks.rearrange.utils.get_camera_transform` #1847

[Bug Fix] Fixing rotational bug in `tasks.rearrange.utils.get_camera_transform` #1847

zephirefaith commented Mar 11, 2024 •

edited

zephirefaith commented Mar 19, 2024 •

edited

aclegg3 left a comment •

edited