More Generic Ortho View Controller

ros-visualization / rviz

ROS 3D Robot Visualizer

BSD 3-Clause "New" or "Revised" License

841 stars 463 forks source link

More Generic Ortho View Controller #1048

Open ajshort opened 8 years ago

ajshort commented 8 years ago

I just open-sourced rviz_ortho_view_controller, which contains an orthographic view controller which isn't locked to a specific plane (unlike the top down ortho view controller). Instead it can free rotate or optionally be locked to a plane if desired.

I was wondering if there would be interest in bringing this kind of thing into RViz itself, since I think it provides a bit more orthographic functionality than what is currently available?

NikolausDemmel commented 8 years ago

Awesome. I would very much welcome the inclusion in rviz proper!

ajshort commented 8 years ago

Cool! Couple of questions:

Would you like to to replace the existing implementation, or add a new one?
Would you prefer a pull request against the jade or kinetic branch?

NikolausDemmel commented 8 years ago

Bear in mind that i am not a maintianer, but here is my opinion:

if it is a superset of the functionality and by default it behaves like the current view controller, updating the existing one makes sense to me
I think a PR against kinetic, which can then be back ported to kinetic and Jade would be best. Or not @wjwwood?

wjwwood commented 8 years ago

Yeah, we can definitely consider taking this upstream. I'd recommend making a pr against kinetic-devel, which is the default branch.

As for extending versus just having a new controller, I'd say it depends on whether or not the existing controller configuration has any benefit (maybe simplicity) over the one you've created. It would be good to get more feedback from others about this particular point.

VictorLamoine commented 8 years ago

Just renaming/updading the class is going to break RViz files:

  Views:
    Current:
      Angle: 0
      Class: rviz/TopDownOrtho

In that case RViz yelds an error but there is no crash, the user just has to select a controller in the list and everything works fine. Try it for yourself, change rviz/TopDownOrtho to rviz/unknown in a config file then load this config file.

I see two possibilities:

Leave it like this (the error is not really pretty...)
Load a default view controller if the view controller could not be parsed (instead of just an error)
Don't rename/delete TopDownOrtho but make sure it calls Ortho with the right parameters (locked in the Z plane by default)

The last solution is the most confortable for the user.

VictorLamoine commented 7 years ago

@ajshort I need orthographic view in RViz, do you plan on integrating your plugin into RViz or not?

If you don't have time or any other reason, do you allow me to create a pull request containing your code (and probably some modifications) into RViz? Of course I want to make sure you are the original author of the code, it needs to be clear.

ajshort commented 7 years ago

@VictorLamoine Unfortunately I haven't had time, so go for it! I've attached a patch which gets the camera working with the current rviz to use as a starting point:

https://gist.github.com/ajshort/87b8bd2a48222436166922bfc8f5272b