Remove keyboard depend by davetcoleman · Pull Request #11 · PickNikRobotics/tf_keyboard_cal

davetcoleman · 2017-05-29T01:49:50Z

Questions for @mcevoyandy:

are there still keyboard shortcuts or is it only by mouse now?
.rviztf is long, why not just .tf?
is the "# (comment line)" because you can't have unlimited comments?
why have IDs in the file format? seems it would just create annoyance?
move the "xyz delta" and "rpy delta" boxes onto the same row to save height in the box.
overall the GUI is kind of tall
it has a ton of debug output, but its not a big deal

davetcoleman

Good looking code!

davetcoleman · 2017-05-29T01:52:06Z

CMakeLists.txt

-    ${PROJECT_NAME}_imarker_simple
+  ${PROJECT_NAME}_gui  
+#    ${PROJECT_NAME}_manual_tf_alignment
+#    ${PROJECT_NAME}_imarker_simple


we still want the ability to use the imarker, right?

davetcoleman · 2017-05-29T01:52:31Z

CMakeLists.txt


-# Library
-add_library(${PROJECT_NAME}_imarker_simple
-  src/imarker_simple.cpp


don't kill this!

davetcoleman · 2017-05-29T01:53:02Z

README.md

+In `rviz` add the tf panel by: `Panel -> Add New Panel ->TFKeyboardCalGUI`
+
+
+


needs to be better integrated into the README

davetcoleman · 2017-05-29T01:53:22Z

frames.gv

@@ -0,0 +1,8 @@
+digraph G {


this and frames.pdf should not be committed

davetcoleman · 2017-05-29T01:53:51Z

include/tf_keyboard_cal/rviz_tf_publisher.h

+
+
+#ifndef RVIZ_TF_PUBLISHER_H_
+#define RVIZ_TF_PUBLISHER_H_


TF_KEYBOARD_CAL_RVIZ_TF_PUBLISHER_H_

davetcoleman · 2017-05-29T01:54:18Z

package.xml

  <license>BSD</license>

  <url type="website">https://github.com/davetcoleman/tf_keyboard_cal</url>
-  <url type="butracker">https://github.com/davetcoleman/tf_keyboard_cal/issues</url>


lol... wow, this is misspelled in my package.xml template I have used for all my many ROS Packages

I noticed a long time ago but it makes me laugh. 'but racker'

Fix eigen warning

mcevoyandy · 2017-05-30T16:13:59Z

@davetcoleman

are there still keyboard shortcuts or is it only by mouse now?

Yes, that Keyboard package is what we were using to listen for key strokes. Not sure if there's something in Qt we could use instead.

.rviztf is long, why not just .tf?

Started with *.tf, but thought not to since tf is a package and might cause confusion... was going to ask your opinion but forgot about it.

is the "# (comment line)" because you can't have unlimited comments?

no limit, just my own file type so wanted to make sure that was noted somewhere. I guess the real answer to this and above is to switch to some other supported file type. yaml?

why have IDs in the file format? seems it would just create annoyance?

I guess it doesn't need to be in there. Can take that out.

move the "xyz delta" and "rpy delta" boxes onto the same row to save height in the box.
overall the GUI is kind of tall

I agree. Open to alternate layouts.

it has a ton of debug output, but its not a big deal

I'll clean up msgs that aren't being used any more or aren't helpful.

mcevoyandy · 2017-05-30T16:16:26Z

@davetcoleman what do you think about the repo name? tf_keyboard_cal seems misleading now that you don't use the keyboard.

davetcoleman · 2017-06-01T00:07:22Z

But you should use the keyboard.... here some example code for Rviz:
https://github.com/davetcoleman/rviz_visual_tools/blob/kinetic-devel/src/key_tool.cpp#L70

I guess the real answer to this and above is to switch to some other supported file type. yaml?

That would be sweet but not super high priority

I guess it doesn't need to be in there. Can take that out.

+1

…nctionality

mcevoyandy · 2017-06-03T06:05:47Z

@davetcoleman I Just added keyboard functionality when on the manipulation tab. I've got my own opinions about functionality/usability, but want to hear what you think before I make any further changes. No doc, but keyboard layout is the same. Save is not functional at the moment.

mcevoyandy · 2017-06-07T05:03:30Z

@davetcoleman added ability to have an imarker on top of tf... so use keyboard, gui, or just drag the marker around. Need help testing and finding cases where stuff doesn't work. Number of possible edge cases is getting hard to manage.

mcevoyandy · 2017-06-21T04:20:16Z

Think I've got all the functionality in there now... gui, keyboard (same buttons as before), interactive markers & menus if you want them. Could you look over and check out the functionality? I'll work on cleaning up and README when I get back to Boulder. Examples of anything you need to load will be in the config folder.

Will also work on the format of the GUI since I shouldn't be adding any more buttons.

davetcoleman · 2017-06-21T06:11:36Z

CMakeLists.txt

-    ${PROJECT_NAME}_imarker_simple
+  ${PROJECT_NAME}_gui
+#    ${PROJECT_NAME}_manual_tf_alignment
+#    ${PROJECT_NAME}_imarker_simple


I think we need to keep the imarker_simple functionality - I believe I am using it in other projects. Perhaps we can deprecate it if you have replacement functionality, but that would still need to be tick-tocked with the next ROS release

We can keep the python version but the old c++ version won't work at all since the keyboard package hasn't been updated.

fyi i've moved the imarker_simple functionality into rviz_visual_tools today, so this functionality is deprecated here

... i recommend you use it for your imarker needs though

davetcoleman · 2017-06-21T06:11:57Z

CMakeLists.txt


-# Library
-add_library(${PROJECT_NAME}_imarker_simple
-  src/imarker_simple.cpp


davetcoleman · 2017-06-21T06:12:24Z

CMakeLists.txt

+  ${catkin_LIBRARIES})
+
+# Library
+# add_library(${PROJECT_NAME}_manual_tf_alignment


if you kill _manual_tf_alignment, just remove all the commented out code

davetcoleman · 2017-06-21T06:12:51Z

README.md

@@ -1,3 +1,13 @@
+# NEW BRANCH


Better documentation than this please

Of course :) Was waiting to finish implementing all the functionality I wanted.

davetcoleman · 2017-06-21T06:13:17Z

frames.gv

@@ -0,0 +1,8 @@
+digraph G {


this file should not be committed

davetcoleman · 2017-06-21T16:11:25Z

src/rviz_tf_publisher.cpp

+
+void RvizTFPublisher::publishTFs()
+{
+  static tf::TransformBroadcaster br;


why are you using a static var here?

might be left over from an initial attempt to get the functionality I wanted. I'll review and remove if not needed anymore.

needs to be there, don't understand why though. comes from their example, without it the TFs don't get published.

davetcoleman · 2017-06-21T16:12:11Z

src/tf_keyboard_cal_gui.h

+namespace tf_keyboard_cal
+{
+struct tf_data{
+  std::size_t id_;


no need for id_?

again, might be left over from something I tried earlier on. Will review and remove if no longer needed.

it's used. I keep it there to generate a unique tf even if the user supplies the same names for "to" and "from"

davetcoleman · 2017-06-21T16:15:43Z

@awesomebytes fyi I think we are going to replace your python-based interactive markers tool with this new approach that also includes that functionality

davetcoleman · 2017-06-21T16:17:25Z

@mcevoyandy I see your point about renaming this package. It hasn't been released since ROS Jade, so for Kinetic and Lunar we could easily rename it. What are you thinking? How about tf_manual_cal? The name is available. This could be a really powerful package

davetcoleman · 2017-06-21T16:33:47Z

@mcevoyandy Questions on latest GUI (just built your PR locally again):

The default .rviz file needs to include the interactive marker display with the correct topic
Somehow I ended up with two interactive markers for the same TF frame
Are the checkboxes necessary for the frames? Why not just have imarkers and menus for all the frames?
Did you add keyboard shortcuts? How do they work?

mcevoyandy · 2017-06-21T16:49:42Z

@davetcoleman

Somehow I ended up with two interactive markers for the same TF frame

I've not noticed anything like this before. I'll see if I can reproduce it.
Are the checkboxes necessary for the frames? Why not just have imarkers and menus for all the frames?

Wasn't sure how to handle this... not all your markers need menus, but each marker that has a menu will have the same menu. We could load imarkers for all frames... could always just turn the topic off in rviz if you didn't want to see them.

Did you add keyboard shortcuts? How do they work?

keyboard shortcuts are there, you have to be on the manipulate tab. same keys as before. Save removed and only contained in the save tab.

I think the basic functionality is there, just need input on matters like these and help looking for any bugs that show up around edge cases that I haven't thought of.

mcevoyandy · 2017-06-28T04:14:15Z

what about tf_visual_tools? Also, need a way to launch from Rviz where the tf publisher also gets launched, not just the GUI interface.

davetcoleman · 2017-06-28T16:56:59Z

I like tf_visual_tools! I'm trying to decide if we should rename this repo or create a new one... I think a new one would be cleanest. Its your codebase at this point so you should keep it under your name (though I'm happy to host it and maintain the release process if you want).

mcevoyandy · 2017-06-28T17:59:41Z

I'd like to learn more about hosting and maintaining for release... not sure what it takes. I'll move the code to a `tf_visual_tools` repo tonight and continue clean up and documentation. Andy McEvoy | (832) 439 - 6150

…

On Wed, Jun 28, 2017 at 10:56 AM, Dave Coleman ***@***.***> wrote: I like tf_visual_tools! I'm trying to decide if we should rename this repo or create a new one... I think a new one would be cleanest. Its your codebase at this point so you should keep it under your name (though I'm happy to host it and maintain the release process if you want). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABSamf7X9685s5KfVm6r9ntCMLnSLXyxks5sIoXbgaJpZM4No1M7> .

davetcoleman · 2017-06-29T00:49:27Z

Cool - if you don't mind I'd still like to be a maintainer / admin of your repo

mcevoyandy · 2017-06-29T01:55:40Z

what do you mean? replace the imarker stuff in tf_keyboard_cal with imarker_simple? Andy McEvoy | (832) 439 - 6150

…

On Wed, Jun 28, 2017 at 6:50 PM, Dave Coleman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In CMakeLists.txt <#11 (comment)> : > LIBRARIES - ${PROJECT_NAME}_manual_tf_alignment - ${PROJECT_NAME}_imarker_simple + ${PROJECT_NAME}_gui +# ${PROJECT_NAME}_manual_tf_alignment +# ${PROJECT_NAME}_imarker_simple ... i recommend you use it for your imarker needs though — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABSamc3Gooc3WiZMxIRpgTnJUKq7ENFaks5sIvTOgaJpZM4No1M7> .

mcevoyandy and others added 22 commits April 25, 2017 22:44

removed keyboard depends

56c43ff

start work on rviz tool

f76dca3

simple panel working

249c622

basic tab layout and create tab

59b3417

format of add/remove tab

a152817

work on manipulation tab

89f76b6

work on increment layout

7896388

added save/load tab, gridlayout on increment tab

b4a2dda

added hooks to remote_receiver

a0f647b

playing with pubs and msgs

181b4e8

work on add/remove comboboxes

63dd0ac

work on populating gui elements with correct data

f29f830

work on update tf msg

47861cd

create/remove/update gui elements working

34d5348

added tf publisher

b5b4ec0

Update README.md

d6a5a18

from dropdown populated properly

103e644

tf listeners setup and working

feb8b18

update readme

4120ba1

load working

3b77953

save working

bfdd0e4

Fix eigen warning

65e5182

davetcoleman commented May 29, 2017

View reviewed changes

Merge pull request #1 from davetcoleman/fix_eigen

1885c3e

Fix eigen warning

mcevoyandy added 2 commits June 1, 2017 22:54

added keyboard listener to manipulate tab, still need to implement fu…

14a0893

…nctionality

keyboard manipulation when manip tab in focus

f10c95b

mcevoyandy added 3 commits June 4, 2017 22:55

start of imarker integration

36058de

tf follows imarker, need to implement reverse

a390e43

imarkers follow gui and keyboard cmds

5c9626b

mcevoyandy added 4 commits June 14, 2017 23:12

fix imarker tf bug

8a570af

.rviztf to .tf and start work on basic imarker right-click menus

4476533

work on creating imarker menus

f480b03

menu pubs done

75875d2

davetcoleman commented Jun 21, 2017

View reviewed changes

begin clean up for new package, begin documentation

349c77c

		In `rviz` add the tf panel by: `Panel -> Add New Panel ->TFKeyboardCalGUI`



		#ifndef RVIZ_TF_PUBLISHER_H_
		#define RVIZ_TF_PUBLISHER_H_

Conversation

davetcoleman commented May 29, 2017

Uh oh!

davetcoleman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcevoyandy commented May 30, 2017

Uh oh!

mcevoyandy commented May 30, 2017

Uh oh!

davetcoleman commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcevoyandy commented Jun 3, 2017

Uh oh!

mcevoyandy commented Jun 7, 2017

Uh oh!

mcevoyandy commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davetcoleman commented Jun 21, 2017

Uh oh!

davetcoleman commented Jun 21, 2017

Uh oh!

davetcoleman commented Jun 21, 2017

Uh oh!

mcevoyandy commented Jun 21, 2017

Uh oh!

mcevoyandy commented Jun 28, 2017

Uh oh!

davetcoleman commented Jun 28, 2017

Uh oh!

mcevoyandy commented Jun 28, 2017 via email

Uh oh!

davetcoleman commented Jun 1, 2017 •

edited

Loading

mcevoyandy commented Jun 21, 2017 •

edited

Loading