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

setup minimal viable rviz2 application #20

Merged
merged 2 commits into from
Sep 27, 2017
Merged

setup minimal viable rviz2 application #20

merged 2 commits into from
Sep 27, 2017

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Sep 7, 2017

The idea here is to get something that compiles, runs, and initializes the render system. I'm commenting out anything remotely difficult to deal with (location of resources, non-trivial uses of ROS 1, etc...) to just get it working.

This should give us a better skeleton to work off of and make improvements to.

This is still a work in progress, opening for visibility so I can sync with my other machine easily.

I've been aggressively adding TODO(wjwwood): ... comments throughout as I go, so I can convert those to issues after I have the application running at least.

@wjwwood wjwwood added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Sep 7, 2017
@wjwwood wjwwood self-assigned this Sep 7, 2017
@wjwwood wjwwood force-pushed the basic_rviz2 branch 2 times, most recently from 76160b6 to 1f6d674 Compare September 14, 2017 01:55
@wjwwood
Copy link
Member Author

wjwwood commented Sep 14, 2017

rebased

wjwwood
wjwwood commented Sep 14, 2017
View reviewed changes
rviz_common/src/rviz_common/display_group.cpp Outdated
@@ -237,7 +237,7 @@ void DisplayGroup::update( float wall_dt, float ros_dt )
Display* display = displays_.at( i );
if( display->isEnabled() )
{
display->update( wall_dt, ros_dt );
display-rviz>update( wall_dt, ros_dt );
Copy link
Member Author

Choose a reason for hiding this comment

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

Man, I was so confused when I got here in my own refactoring 😄.

@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 26, 2017
@wjwwood
Copy link
Member Author

wjwwood commented Sep 26, 2017

Ok, I've rebased this into two commits (one moving files and the other all the other changes). I'll try to come up with a list of pointers to make the review easier after lunch.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 26, 2017

Some notes for reviewers:

What I'd like out of a review:

  • find places where typos were made
  • find inconsistencies in the migration or updating of files which have not been marked with a TODO
    • basically anything suspicious that hasn't been marked with a TODO for tracking
    • everything in the rviz_common/src/rviz_common/temp folder is exempt from this because I consider the whole folder a TODO 😄
  • ideas or questions about new things (like the ros_integration stuff)

Other things:

  • 5c399aa is only moving files
  • fa7ac92c17267958dbf48fe30aa156fe160bff86 are all the substantive changes
  • not all the files will pass the linters (this will be ticketed)
  • this will probably not build on Linux and/or Windows without some extra dependencies which are not yet documented (will be ticketed)

While the reviews are happening (I hope to merge this tomorrow) I will be auditing the code to make issues for as many of the TODO's as is possible.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 26, 2017

Also, this pr requires ros2/rclcpp#375 which was merged after beta-3, so if you want to test this you'll need the latest from the ros2 repositories.

@wjwwood
Copy link
Member Author

wjwwood commented Sep 26, 2017

Oh, and for those that didn't see it, this is what could be rendered with this branch if you had the right setup and associated programs running:

https://gfycat.com/formaldownrightindigowingedparrot

I won't take the time to describe that setup, since it was so fragile. A better (easier to setup) demo is forthcoming.

greimela-si
greimela-si reviewed Sep 27, 2017
View reviewed changes
rviz_common/src/rviz_common/temp/resource_retriever/retriever.h
: size(0)
{}

std::shared_ptr<uint8_t> data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Import <memory> when using std::shared_ptr.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add it, but this file (since it is in the temp folder) will probably be deleted in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, but the error would persist if the file is moved instead.

@wjwwood wjwwood merged commit ca8f42e into ros2 Sep 27, 2017
@wjwwood wjwwood deleted the basic_rviz2 branch September 27, 2017 21:57
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Sep 27, 2017
This was referenced Sep 27, 2017
Closed
Merged
@hidmic hidmic mentioned this pull request Oct 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greimela-si greimela-si greimela-si left review comments

Assignees

@wjwwood wjwwood

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wjwwood @greimela-si