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

Fix issues #113

Merged
merged 14 commits into from
Dec 16, 2022
Merged

Fix issues #113

merged 14 commits into from
Dec 16, 2022

Conversation

HoangGiang93
Copy link
Contributor

I fixed most of the issues that @daniel86 addressed. Can you check on that?

@daniel86
Copy link
Contributor

daniel86 commented Oct 13, 2022

@HoangGiang93 could you maybe summarize your changes? especially what are the new link names? and did you refactor others? I would need to go through the whole file again without this summary.

btw. why is the diff so large? It is odd, e.g. the diff states you removed 1,787 lines from kitchen.xacro (you deleted the file) but in the same diff it is added again with 1855 lines.
What is expected to see a diff with just the roughly 100 lines that changed. that would also be a bit more comprehensible for doing a review ;) maybe you messed up something?

@daniel86 daniel86 self-requested a review October 13, 2022 16:29
Copy link
Contributor

@daniel86 daniel86 left a comment

Choose a reason for hiding this comment

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

the diff is huge. it contains the whole URDF file. Did something went wrong? Could you have a look into that the diff only includes your changes?

@HoangGiang93
Copy link
Contributor Author

HoangGiang93 commented Oct 13, 2022

Sorry I @daniel86, I tried different ways to use git mv to rename the file but it didn't work properly. After I modify the file (the old one but with a new name), github always change it into deleting that file and creating a new one :/

image

You see, I tried to rename apartment.urdf to furnitures.xacro. Then I modified furnitures.xacro and pushed it. Github changed it into deleting apartment.urdf and creating furnitures.xacro, which removed all the old commits from the others.

@HoangGiang93
Copy link
Contributor Author

I think if you want to know which code I changed that actually fixed the issue, you can go through each commit. Before the commit f410487 that fixes the issue #106, I didn't rename any files, so you would see the diff there better. We can discuss about this later.

@daniel86
Copy link
Contributor

Seems fine to me. I think one of the persons using the URDF on the CRAM side should approve as well. maybe @artnie ?

@daniel86 daniel86 requested a review from artnie October 16, 2022 10:42
@artnie artnie removed their request for review October 17, 2022 08:58
@gaya-
Copy link
Member

gaya- commented Oct 17, 2022

I can't see if the coordinates are still the same or different. For example, wall_coloksu_wall1_joint changed coordinates. Was that on purpose or a copy paste mistake from a previous URDF? It would be great if somebody spawned the 2 kitchens half-transparent at the same time, then we'd see if the offsets are still fine. This can be done with the bullet world by having two different names for the kitchens. Or in Giskard PyBullet. Or maybe even in RViz. It would be great if indentation changes were committed separately, so one could look at apartment.urdf and see what changed. So the urdf -> xarco change should've been it's own commit. @HoangGiang93 where is the commit that changed the wall_coloksu_wall1_joint origin?

@HoangGiang93
Copy link
Contributor Author

It would be great if indentation changes were committed separately, so one could look at apartment.urdf and see what changed.

I changed the name of kitchen.xarco to kitchen.xacro in 1 commit and github still turned it into deleting and creating :(

@HoangGiang93 where is the commit that changed the wall_coloksu_wall1_joint origin?

I didn't change the wall_coloksu_wall1_joint origin. See below:
https://github.com/code-iai/iai_maps/blob/master/iai_apartment/urdf/apartment.urdf#L53
https://github.com/code-iai/iai_maps/blob/FixIssues/iai_apartment/urdf/furnitures.xacro#L50

@gaya-
Copy link
Member

gaya- commented Oct 17, 2022

I didn't change the wall_coloksu_wall1_joint origin. See below: https://github.com/code-iai/iai_maps/blob/master/iai_apartment/urdf/apartment.urdf#L53 https://github.com/code-iai/iai_maps/blob/FixIssues/iai_apartment/urdf/furnitures.xacro#L50

Indeed, you didn't. In the diff it looked like you did.
Ok, so from what I can see the offsets are ok.

@sunava Maybe you could try to pull this PR onto pr2-ext and see if the demo still runs?
Of course making sure to note the commit that we're using now in case you need to revert.

Asking @sunava because she is showing the demo to our visitors and will be very angry if anyone else breaks the demo. :)

@gaya-
Copy link
Member

gaya- commented Oct 20, 2022

When I start the upload_kitchen.launch file

$ roslaunch iai_apartment apartment_bringup.launch 

I get the following errors:

process[iai_apartment_root_link_broadcaster-1]: started with pid [5726]
process[apartment_state_publisher-2]: started with pid [5727]
process[apartment_joint_state_publisher-3]: started with pid [5738]
[ERROR] [1666284384.401244446]: material 'oven_gray' is not unique.
[apartment_state_publisher-2] process has died [pid 5727, exit code 255, cmd /opt/ros/kinetic/lib/robot_state_publisher/robot_state_publisher robot_description:=apartment_description joint_states:=apartment_joint_states __name:=apartment_state_publisher __log:=/home/gaya/.ros/log/51aff9c0-5096-11ed-abb3-e03f4977eaf0/apartment_state_publisher-2.log].
log file: /home/gaya/.ros/log/51aff9c0-5096-11ed-abb3-e03f4977eaf0/apartment_state_publisher-2*.log
Traceback (most recent call last):
  File "/opt/ros/kinetic/lib/joint_state_publisher/joint_state_publisher", line 72, in <module>
    jsp.loop()
  File "/opt/ros/kinetic/lib/python2.7/dist-packages/joint_state_publisher/__init__.py", line 274, in loop
    msg.position[i] = joint['position'] * factor + offset
IndexError: list assignment index out of range
[apartment_joint_state_publisher-3] process has died [pid 5738, exit code 1, cmd /opt/ros/kinetic/lib/joint_state_publisher/joint_state_publisher robot_description:=apartment_description /joint_states:=/apartment_joint_states __name:=apartment_joint_state_publisher __log:=/home/gaya/.ros/log/51aff9c0-5096-11ed-abb3-e03f4977eaf0/apartment_joint_state_publisher-3.log].
log file: /home/gaya/.ros/log/51aff9c0-5096-11ed-abb3-e03f4977eaf0/apartment_joint_state_publisher-3*.log

The main problem seems to be the following line:

[ERROR] [1666284384.401244446]: material 'oven_gray' is not unique.

Please fix.

@gaya-
Copy link
Member

gaya- commented Oct 20, 2022

And please let me know if you don't have this problem. It's weird that two people approve of this PR whereby the basic launch already gives errors.

@gaya-
Copy link
Member

gaya- commented Oct 20, 2022

Then when I use my own launch file, I get

Link `side_A_rb' seems to have two parents

How did you guys test this?

@HoangGiang93
Copy link
Contributor Author

When I start the upload_kitchen.launch file

$ roslaunch iai_apartment apartment_bringup.launch 

I get the following errors:

process[iai_apartment_root_link_broadcaster-1]: started with pid [5726]
process[apartment_state_publisher-2]: started with pid [5727]
process[apartment_joint_state_publisher-3]: started with pid [5738]
[ERROR] [1666284384.401244446]: material 'oven_gray' is not unique.
[apartment_state_publisher-2] process has died [pid 5727, exit code 255, cmd /opt/ros/kinetic/lib/robot_state_publisher/robot_state_publisher robot_description:=apartment_description joint_states:=apartment_joint_states __name:=apartment_state_publisher __log:=/home/gaya/.ros/log/51aff9c0-5096-11ed-abb3-e03f4977eaf0/apartment_state_publisher-2.log].
log file: /home/gaya/.ros/log/51aff9c0-5096-11ed-abb3-e03f4977eaf0/apartment_state_publisher-2*.log
Traceback (most recent call last):
  File "/opt/ros/kinetic/lib/joint_state_publisher/joint_state_publisher", line 72, in <module>
    jsp.loop()
  File "/opt/ros/kinetic/lib/python2.7/dist-packages/joint_state_publisher/__init__.py", line 274, in loop
    msg.position[i] = joint['position'] * factor + offset
IndexError: list assignment index out of range
[apartment_joint_state_publisher-3] process has died [pid 5738, exit code 1, cmd /opt/ros/kinetic/lib/joint_state_publisher/joint_state_publisher robot_description:=apartment_description /joint_states:=/apartment_joint_states __name:=apartment_joint_state_publisher __log:=/home/gaya/.ros/log/51aff9c0-5096-11ed-abb3-e03f4977eaf0/apartment_joint_state_publisher-3.log].
log file: /home/gaya/.ros/log/51aff9c0-5096-11ed-abb3-e03f4977eaf0/apartment_joint_state_publisher-3*.log

The main problem seems to be the following line:

[ERROR] [1666284384.401244446]: material 'oven_gray' is not unique.

Please fix.

image

Sorry I don't have that error :(

@HoangGiang93
Copy link
Contributor Author

The main problem seems to be the following line:

[ERROR] [1666284384.401244446]: material 'oven_gray' is not unique.

And by the way the oven_gray is actually unique https://github.com/code-iai/iai_maps/blob/FixIssues/iai_apartment/urdf/apartment.urdf#L535

@sunava
Copy link
Contributor

sunava commented Oct 24, 2022

i can try Tuesday if that works @gaya-

@daniel86
Copy link
Contributor

i can try Tuesday if that works @gaya-

so?

@sunava
Copy link
Contributor

sunava commented Nov 16, 2022

@gaya- @HoangGiang93 Sadly the test did not went well, the robot is driving against the wall when trying to open the door. Maybe that has something to do with the orientation changes that u mentioned gaya? Idk if you guys want me to investigate this more or not

@HoangGiang93
Copy link
Contributor Author

@gaya- @HoangGiang93 Sadly the test did not went well, the robot is driving against the wall when trying to open the door. Maybe that has something to do with the orientation changes that u mentioned gaya? Idk if you guys want me to investigate this more or not

I will need more information to know where the problem is

@ichumuh
Copy link
Contributor

ichumuh commented Dec 8, 2022

@sunava are you sure you loaded the kitchen into giskard?

@sunava
Copy link
Contributor

sunava commented Dec 9, 2022

since this happened last week to Alina, im going to check it on Tuesday if that was the issue. I remember to try it 5 times tho would be a bigger if i failed to load it correctly all the time :D, will update u

@sunava
Copy link
Contributor

sunava commented Dec 14, 2022

@HoangGiang93 @daniel86 approved from myside

@HoangGiang93 HoangGiang93 merged commit 975d6b4 into master Dec 16, 2022
@gaya- gaya- deleted the FixIssues branch April 2, 2023 19:44
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.

5 participants