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.

Sign up for GitHub

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

Jump to bottom

Migrate Pose Display #204

Merged
merged 7 commits into from
Mar 8, 2018
Merged

Migrate Pose Display #204

merged 7 commits into from
Mar 8, 2018

Conversation

Martin-Idel-SI
Copy link
Contributor

@Martin-Idel-SI Martin-Idel-SI commented Feb 7, 2018 •
edited
Loading

Resolves #86

@Martin-Idel-SI
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@greimela-si
Copy link
Contributor

greimela-si commented Mar 6, 2018 •
edited
Loading

Rebased after changes on ros2 branch.

New CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status (failures unrelated)

@wjwwood wjwwood added the in review Waiting for review (Kanban column) label Mar 8, 2018
wjwwood
wjwwood approved these changes Mar 8, 2018
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

@wjwwood wjwwood merged commit af74129 into ros2:ros2 Mar 8, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Mar 8, 2018
@sloretz
Copy link
Contributor

sloretz commented Mar 8, 2018 •
edited
Loading

Missing header arrow.hpp? I got this error in a CI job

23:12:52 /home/rosbuild/ci_scripts/ws/src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/displays/pose/pose_display.cpp:36:36: fatal error: rviz_rendering/arrow.hpp: No such file or directory

Edit: failing build

edit 2: I don't see arrow.hpp in the rviz_rendering package include folder

@sloretz
Copy link
Contributor

sloretz commented Mar 9, 2018

Also missing axes.hpp. Should the include be <rviz_rendering/objects/axes.hpp> and <rviz_rendering/objects/arrow.hpp> instead?

23:11:51 /home/rosbuild/ci_scripts/ws/src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/displays/pose/pose_display_selection_handler.cpp:42:35: fatal error: rviz_rendering/axes.hpp: No such file or directory

@wjwwood
Copy link
Member

wjwwood commented Mar 9, 2018

Weird, I built this locally, I guess the conflict resolution messed up somewhere. Let me look real quick.

wjwwood added a commit that referenced this pull request Mar 9, 2018
@wjwwood
Revert "Migrate Pose Display (#204)"
4a32005 
This reverts commit af74129.
@wjwwood wjwwood mentioned this pull request Mar 9, 2018
Merged
wjwwood added a commit that referenced this pull request Mar 9, 2018
@wjwwood
Revert "Migrate Pose Display (#204)" (#227)
b209e13 
This reverts commit af74129.
@wjwwood
Copy link
Member

wjwwood commented Mar 9, 2018

I went ahead and reverted it until I can figure out what went wrong, sorry for the hiccup.

#227

wjwwood added a commit that referenced this pull request Mar 9, 2018
@wjwwood
Revert "Revert "Migrate Pose Display (#204)" (#227)"
c775ae7 
This reverts commit b209e13.
@wjwwood
Copy link
Member

wjwwood commented Mar 9, 2018

@sloretz it was this other pr that moved the file, so individually they passed CI, but after merging both this one was broken:

https://github.com/ros2/rviz/pull/210/files#diff-d542aecef54d76e5d18642037240f2fd

I'll fix it up and run new CI for this one.

@sloretz
Copy link
Contributor

sloretz commented Mar 9, 2018

Thanks @wjwwood

Hard to spot a move in a +4800,-8200 😮 PR

@wjwwood
Copy link
Member

wjwwood commented Mar 9, 2018

Well, and I built it locally, but not after merging the other pr. I guess technically we should rerun CI after every commit to the target branch (ros2 in this case).

@wjwwood wjwwood mentioned this pull request Mar 9, 2018
Merged
@Martin-Idel-SI Martin-Idel-SI deleted the feature/migrate_pose_display branch March 9, 2018 09:02
wjwwood added a commit that referenced this pull request Mar 12, 2018
@wjwwood
Revert "Revert "Migrate Pose Display"" (#228)
8dfb0c0 
* Revert "Revert "Migrate Pose Display (#204)" (#227)"

This reverts commit b209e13.

* fix include statements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Martin-Idel-SI @greimela-si @sloretz @wjwwood