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

relocate all the files needed by VisualizerApp #19

Merged
merged 6 commits into from
Sep 6, 2017
Merged

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Sep 6, 2017

While working on #14 I realized I was iterating on dead code from rviz (the camera stuff in ogre_helpers is no longer used AFAICT), so I decided to take a different approach and do a top down move of all the files I think we'll need to port for the rviz application (does not include what plugins might use).

So I moved all the files needed by the VisualizerApp which is the main entry point for the rviz application into either rviz_rendering or the new package I've called rviz_common which will contain everything not specific to Qt-Ogre integration or basic rendering and the very highest level entry points which will live in rviz and rviz2 for ROS 1 and ROS 2 respectively.

I did this by doing a depth first crawl of the include statements, moving all headers to either rviz_rendering (defaulting to public headers, i.e. into the include directory) or rviz_common (defaulting to private, i.e. into the src directory). I figure we can move files more later, but this way we have to opt into public headers which will help us keep a smaller public API which will help with API and ABI compatibility down the road.

In the process of moving I changed the header file extensions to be hpp rather than h, but other than than the files are unchanged. I also added an entry for all source files (cpp) in the CMakeLists.txt, but commented out so that we can turn them on a few at a time as we iterate on the main Qt application and port more stuff (we'll still have to remove/replace ROS 1 stuff and uses of pluginlib/boost/etc...). I also added entries for header files which I thought might need to be AUTOMOC'ed, again commented out to begin with.

I think we should merge this pr as-is, knowing that we will need to change this stuff later, for example we might:

  • move headers to make them public/private (e.g. to/from the include and src folders)
  • get rid of headers/source files that are no longer needed
  • split files up to make part public and part private
  • reorganize to have more hierarchy in the header structure, e.g. I'd like to group the "panels" in their own folder to make it easier to navigate
  • etc...

I'm also hoping that this will let us parallelize more and also to avoid a situation where multiple pr's are moving the same files and stuff like that.

Also, this is not all of the files, since I only went recursively through the VisualizerApp I haven't move files that might only be used by the plugins, but I figure I do that in a separate pr. In the end we'll only be left with dead code or really ROS 1 specific code in the rviz package.

I calculated this list by doing a depth first crawl of the include
statements in both the headers and c++ files. I moved the headers to
either rviz_rendering (defaulting to public headers) or rviz_common
(defaulting to private headers) and changed the extensions to hpp. I
also moved the cpp files to the respective package. I also added a line
in the CMakeLists.txt for all the source files and also the headers if I
determined that they would need Qt's automoc at a glance. I added those
entried commented out so they could be turned on a few at a time when we
start fixing up the code. Aside from changing the extensions on the
header files, the content of the files is untouched.
@wjwwood wjwwood added the enhancement New feature or request label Sep 6, 2017
@wjwwood wjwwood self-assigned this Sep 6, 2017
@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Sep 6, 2017
@wjwwood
Copy link
Member Author

wjwwood commented Sep 6, 2017

Responding to #14 (comment), which I think was meant for this repository.

The linters cpplint and uncrustify fail for rviz_rendering. For rviz_common the linters do not run for me using ament test --build-tests --only rviz_common --cmake-args -DCMAKE_BUILD_TYPE=Release.

Yeah, this is part of what I was talking about with incremental changes. Unfortunately there is no way to easily tell the linters to ignore these new files until we have time to fix them up. I would blacklist them in the linters temporarily if I could do that.

I'll look at why the linter doesn't run on rviz_common.

I would prefer to fix at least the linter errors in rviz_rendering.

I can update the files moved to rviz_rendering, at least, but I would like to leave rviz_common until we get the rviz app working (with stuff commented out, etc...).

@wjwwood
Copy link
Member Author

wjwwood commented Sep 6, 2017

I pushed 1b5487c which lets the linters pass on rviz_rendering for me.

rviz_common's linters run, but obviously fail with lots of errors since the new files have not had the new styles applied.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 6, 2017

I also looked at passing a whitelist or blacklist to the linters, and for both I think we could do a whitelist, but I think that would be pretty inconvenient. Also, I think cpplint could only do whitelisted folders, not files.

Anyways, as bad as it is to have failing linters for now, I think it's best to merge this and iterate on the files as needed.

I will merge this now and then start hacking until I get something working for the "rviz2" application, commenting out stuff and noting them as I go. After that I will make issues for as many of the commented out sections as is applicable, then we can start to burn those issues down.

@wjwwood wjwwood merged commit 07054d1 into ros2 Sep 6, 2017
@wjwwood wjwwood deleted the the_great_migration branch September 6, 2017 21:47
@wjwwood wjwwood removed the in progress Actively being worked on (Kanban column) label Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant