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

Add symmetries publisher and publish empty messages on no detections #1

Merged
merged 34 commits into from
Sep 23, 2024

Conversation

Kotochleb
Copy link
Collaborator

No description provided.

@Kotochleb Kotochleb marked this pull request as ready for review July 18, 2024 09:53
Copy link
Contributor

@MedericFourmy MedericFourmy left a comment

Choose a reason for hiding this comment

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

Seems fine. I wonder how we could add the possibility of asking for discretized continuous symmetries using RigidObject.make_symmetry_poses. Or to simplify things, we may assume that the end user can do it itself.

@@ -65,6 +66,17 @@ def update_params(self, params: dict) -> None:
else None
)

def get_dataset(self) -> RigidObjectDataset:
"""Returns rigid object dataset used by detector.
Copy link
Contributor

Choose a reason for hiding this comment

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

Returns rigid object dataset used by happypose pose estimator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

def get_dataset(self) -> RigidObjectDataset:
"""Returns rigid object dataset used by detector.

:return: Dataset used by detector.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rigid object dataset used by pose estimator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

:rtype: happypose_msgs.msg.ContinuousSymmetry
"""
return ContinuousSymmetry(
# Explicit conversion to float is required in this case
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added more explanation in the comment. Should be clearer now

def discrete_symmetry_to_msg(
symmetry: happypose_symmetries.DiscreteSymmetry,
) -> Transform:
"""Converts HappyPose DiscreteSymmetry object into Transform ROS message corresponding to
Copy link
Contributor

Choose a reason for hiding this comment

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

end of the sentence? Would just end at "...into Transform ROS message."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. I fixed it

) -> Transform:
"""Converts HappyPose DiscreteSymmetry object into Transform ROS message corresponding to

:param symmetry: HappyPose object storing definition of continuous symmetry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Of "discrete symmetry"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@Kotochleb
Copy link
Collaborator Author

I started adding a helper function in happypose_msgs package that will allow users to discretize the continuous symmetries. Current API assumes ObjectSymmetries message is passed and function returns tensor with transformation matrices. This function is partially reimplementation of built-in HappyPose function since messages package shoudl be installable without installation of whole HappyPose and its dependencies

@Kotochleb Kotochleb requested a review from MedericFourmy July 23, 2024 13:22
happypose_msgs/msg/ObjectSymmetriesArray.msg Outdated Show resolved Hide resolved
happypose_msgs/msg/ObjectSymmetries.msg Outdated Show resolved Hide resolved
happypose_ros/.gitignore Outdated Show resolved Hide resolved
happypose_ros/.gitignore Outdated Show resolved Hide resolved
happypose_ros/.gitignore Outdated Show resolved Hide resolved
@@ -98,17 +121,17 @@ def test_03_trigger_pipeline(self, proc_output: ActiveIoHandler) -> None:
ready = proc_output.waitFor("HappyPose initialized", timeout=0.5)
assert ready, "Failed to trigger the pipeline!"

def test_04_receive_messages(self) -> None:
def test_06_receive_messages(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why numbering?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It enforces order of the unit tests as they are executed alphabetically. This is due to the sequential nature of the way the ROS node works and in this case, actions have to be executed in a certain order to avoid repeating of computations, which are costly in this case

@Kotochleb
Copy link
Collaborator Author

The ability to discretize continuous symmetries is finally implemented. It turned out the original implementation from HappyPose had numerous bugs and was incredibly inefficient. This implementation should work better and is tested. Though, I would like someone to take a look at it and see if I didn't make mistakes in the implementation, even at the stage of unit testing. @MedericFourmy could you take a look at is as it was done per your request?

Copy link
Contributor

@MedericFourmy MedericFourmy left a comment

Choose a reason for hiding this comment

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

implementation looks ok, offset definition should be verified with a 3D viz tool

out = np.zeros((n_con + n_disc + n_mix, 4, 4))

# Precompute steps of rotations
rot_base = 2.0 * np.pi / n_symmetries_continuous
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:
discretized_angles = np.linspace(0, 2*np.pi, n_symmetries_continuous)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

np.linspace will include 2 pi in the resulting list, creating redundant value.
Code below compares results:

n_symmetries_continuous = 6
rot_base = 2.0 * np.pi / n_symmetries_continuous

lin_space = np.linspace(0.0, 2.0 * np.pi, n_symmetries_continuous)
manual = np.array([rot_base * j for j in range(n_symmetries_continuous)])

print(f"Linspace: {np.rad2deg(lin_space)}")
print(f"Manual:   {np.rad2deg(manual)}")

With an output

Linspace: [  0.  72. 144. 216. 288. 360.]
Manual:   [  0.  60. 120. 180. 240. 300.]

This can be equal if line

lin_space = np.linspace(0.0, 2.0 * np.pi, n_symmetries_continuous)

was replaced with

lin_space = np.linspace(0.0, 2.0 * np.pi, n_symmetries_continuous+1)[:-1]

But to be fair, this makes it even less readable.

I would honestly leave it as it is

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 this:

np.linspace(0,10, 10, endpoint=False)
>> array([0., 1., 2., 3., 4., 5., 6., 7., 8., 9.])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I ended up with the linspace version as requested

end = (i + 1) * n_symmetries_continuous
out[begin:end, :3, :3] = np.array(
[
transforms3d.axangles.axangle2mat(axis, rot_base * j)
Copy link
Contributor

Choose a reason for hiding this comment

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

then
discretized_angles[j]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above

@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.5.0)
cmake_minimum_required(VERSION 3.10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the version of CMake that is the default on Ubuntu LTS 22.04?

Copy link

Choose a reason for hiding this comment

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

@@ -0,0 +1,45 @@
cmake_minimum_required(VERSION 3.10)
Copy link
Contributor

Choose a reason for hiding this comment

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

CMake version on Ubuntu 22.04?
This code targets this version at the minimum so it should ask for this CMake version at the minimum.

cmake_minimum_required(VERSION 3.10)
project(happypose_msgs)

find_package(ament_cmake REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

For future reference, concerning pure ament package like this please consider ament_cmake_auto.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Due to an additional package happypose_msgs_py being created on a build time, I decided to resort to default ament

@Kotochleb Kotochleb merged commit 3008139 into main Sep 23, 2024
2 checks passed
@Kotochleb Kotochleb deleted the feature/publishing-symmetries branch September 23, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants