opendr icon indicating copy to clipboard operation
opendr copied to clipboard

ros1_node for landmark_expression_recognition updated

Open negarhdr opened this issue 3 years ago • 4 comments

This branch fixes the issues of ROS1 node for landmark_based_facial_expression_recognition tool.

negarhdr avatar Sep 14 '22 08:09 negarhdr

As a general comment, i tried to run the node even though i don't have the pretrained models. Running the node threw an error:

Running script landmark_based_facial_expression_recognition.py...
[rosrun] Couldn't find executable named landmark_based_facial_expression_recognition.py below ~/opendr/projects/opendr_ws/src/perception
[rosrun] Found the following, but they're either not files,
[rosrun] or not executable:
[rosrun]   ~/opendr/projects/opendr_ws/src/perception/scripts/landmark_based_facial_expression_recognition.py

I resolved by adding the script here, running catkin_make install and then source install/setup.bash instead of source devel/setup.bash. Then the node fails when it can't find the model as expected.

This might have revealed a bigger issue with the ROS1 package and its usage, as only some of the nodes are listed in CMakeLists.txt and the instructions read to just run catkin_make and source devel/setup.bash.

I ran into this issue today while trying to test this node, up until yesterday all nodes ran successfully by following the instructions in the readme. Nodes that ran locally yesterday (and are included in CMakeLists.txt), such as pose estimation, were throwing import errors today, because they used the local python interpreter instead of venv (shebang was ignored as explained here and here). The solution was the same (catkin_make install source install/setup.bash).

@stefaniapedrazzi, @passalis what do you think about this? Should i open an issue documenting what i found, so we can independently test it? Could it be a local issue on my machine?

tsampazk avatar Sep 14 '22 12:09 tsampazk

@stefaniapedrazzi, @passalis what do you think about this? Should i open an issue documenting what i found, so we can independently test it? Could it be a local issue on my machine?

A colleague independently did a fresh install of ros and ran the nodes as documentation suggests and they ran ok. The issue seems to be on my machine, so i will do a fresh install and investigate further. Right now i can only say that the issue appeared when i checked out Negar's branch and tried to test the node, seemingly through no fault of the changes in this branch.

tsampazk avatar Sep 15 '22 10:09 tsampazk

Please also check this branch if this is ready to be merged. Thanks.

negarhdr avatar Oct 19 '22 11:10 negarhdr

Hey thanks @negarhdr, i will add a new review. Until i do, could you please take a look at the pep8 errors that fail the test?

tsampazk avatar Oct 19 '22 11:10 tsampazk

Thanks @negarhdr! Node looks good, apart from a couple of minor things.

Thanks! The comments are addressed.

negarhdr avatar Oct 21 '22 19:10 negarhdr

In regards to the issue stated above, there's a relevant discussion here.

tsampazk avatar Oct 25 '22 10:10 tsampazk