search

graspit-simulator / graspit

The GraspIt! simulator
http://graspit-simulator.github.io/
Other
180 stars 81 forks source link

Abstract IVmgr for headless mode #38

Closed JenniferBuehler closed 8 years ago

JenniferBuehler commented 8 years ago

I've added a simple base class IVmgrBase which is needed to provide different implementations of the IV manager functionality. There are two classes deriving from it:

Changes which were required

JenniferBuehler commented 8 years ago

Thanks Matei for the feedback. I'll add the changes within the next day.

jvarley commented 8 years ago

We can just remove MAX_COLLISIONS and MAX_POLYTOPES, I grepped for them. They don't appear outside their definition. I am going to look at this PR more today.

jvarley commented 8 years ago

2 thoughts:

1)Can we add a new command line option to enable graspit to be run in a headless mode? I feel a bit weary about adding new files that aren't used by GraspIt in a standalone mode. Also, I would really like to be able to use this.

2) What do you guys think of: ivmgrbase -> IVManager ivmgr -> InteractiveIVManager ivmgrHeadless -> HeadlessIVManager

mateiciocarlie commented 8 years ago

Sounds good to me...

JenniferBuehler commented 8 years ago

Agreed. I've added the changes. Love travis btw :+1:

JenniferBuehler commented 8 years ago

Question about comment by @jvarley about adding a command line option: I have only used the headless mode for the EGPlanner. There's a few more classes involved around accessing the graspit world... can you describe what you have in mind in a bit more detail please? I'll see what I can add to the PR in order to make use of the headless mode.

For your reference: I'm using the headless mode from class GraspItSceneManagerHeadless.

jvarley commented 8 years ago

So I pretty much exclusively use graspit by writing my own custom plugins. It is very easy to make plugins that are their own ros package. For example the graspit_moveit_plugin.

I have a plugin that generates grasps to use as training data for learning algorithms. It loads an object, runs a planner, and uploads the results to Mongo. I would love it to be able to run this without graspit constantly poping up on my screen. I currently have a separate computer that I run this on due to this issue.

The plugin is here: https://github.com/CURG/graspit_data_gen

So I run ./graspit_simulator -p pluginName

I would like to run: ./graspit_simulator -p pluginName -headless

Then the plugin will do whatever it needs to, and it will all run in headless mode.

jvarley commented 8 years ago

Would it make sense to convert your graspit-planning-graspit library into a graspit plugin? Or if that won't work, maybe we could explore why so that we can standardize/improve how custom code integrates with graspit.

JenniferBuehler commented 8 years ago

Hmmm I hadn't thought to use a plugin, would have to look at it when I get the time (hopefully on the weekend or next week). Is it possible to load a plugin before the main GraspIt framework is loaded (ie. before IVManager is created)?

jvarley commented 8 years ago

Currently ivmgr is created before the plugins are loaded, but I think we could move the processing of args to before the creation of the ivmgr. Then we could modify how ivmgr is created based on the args. Currently the plugins are allocated and initialized pretty late in the startup process, but I think we could revisit that.

The args are processed and the plugins are loaded in: https://github.com/graspit-simulator/graspit/blob/master/src/graspitGUI.cpp

JenniferBuehler commented 8 years ago

Ok, that's surely worth looking into. I'm currently helping to fix other issues as well so not sure when I'll get around to look at it, hopefully over the weekend. If you get any new insights or ideas in this regard, let me know!

jvarley commented 8 years ago

I think we could make a graspit flag to determine what type of ivmanager to load. Then we could put the rest of your code in a plugin that can be loaded at a later point. I will look into it a bit as well, I haven't run your planning code before so I will download/try it to get a better feel for it.

mateiciocarlie commented 8 years ago

Thanks for all the work and discussion. I think it highlights some pretty fundamental architectural decisions that could be improved... However, maybe it's best to do some incremental steps.

Ultimately, the model where a lot of "usage" of GraspIt as an engine happens in plugins is great. Moving wholesale to that model though will be hard. Maybe the right solution is for the InteractiveIVManager to be a plugin itself.

The problem now is that some concepts which should be separated are combined together. The IVMgr not only handles user interaction, but is used in many places as a "gateway" to the GraspIt engine, as it holds a pointer to the world. No need for those two concepts to be combined. Furthermore, the Inventor scene is used both for rendering, and for keeping track of what's where in the world.

@JenniferBuehler has taken many steps towards disentangling all these concepts. Is it maybe the case that this PR could be merged, leaving for future discussion the following:

JenniferBuehler commented 8 years ago

Agreed, the fact that the IVmgr manages the world as well is a bit of a stumble stone... but the IVManager (or rather, whatever manages the the main Qt thread) and the WorldElement objects are related because of the QObject inheritance. All QObjects are strongly bound to the Qt thread (eg. they cannot be created/deleted/manipulated outside the Qt thread). I've had a lot of warnings and errors due to this, until I finally managed to run everything from a new, instead of the main thread, without everything crumbling. This concept is something I'm not too fond of in the Qt framework itself, though I'm sure there are some good reasons for having it this way.

Agreed also that we should do this step-by-step. The first step, as it is now in this PR, is only minimal (despite many renames now due to the new class names). We can then build it up from there and look into adding the option to run plugins in headless mode as well, which would definitely be very useful for all external code.

jvarley commented 8 years ago

I completely agree with an incremental approach, but I am still not convinced this is quite ready to be pulled in. I am very hesitant to add code into graspit that has no use by graspit, and is only used by external projects. I think this will lead to confusion. If we implement the headless mode as a graspit arg, then this code will have a use within graspit, and I think we are only a few hours of work away from having this. I will spend a few hours tonight and see if I can implement the headless flag.

The incremental steps that I see occurring after this are: 1) establish a better entry point to the graspit engine that is not IVmgr. 2) find a better mechanism to enforce that all world elements are created by the same thread 3) Move more things to plugins. (planner, ivmgr)

jvarley commented 8 years ago

Also props to @JenniferBuehler for doing the work on this. I really do want to have this feature available in a stable manner.

jvarley commented 8 years ago

Hi Guys,

Sorry this is a long one. If you both think it is worth pulling in the current PR and then addressing this as a separate PR, then I can drop it. But as it stands, I personally think we are close enough to fixing this in a more substantial way that we ought to wait on pulling this in.

I created a branch that runs graspit in a headless mode with a plugin. https://github.com/jvarley/graspit/tree/headless this is NOT an alternative to what @JenniferBuehler has proposed. I want to find a way to combine them . The branch that I created leaves ivmgr filled with null pointers as it basically just skips the initialization of a bunch of things if it is in headless mode, so we definitely want to have different implementations of IVManager. I just think it would be nice to combine these two use cases before we pulled anything in.

My branch does well: Parses args outside of graspitGUI and is able to predicate how ivmanager is created based on args My branch does poorly: Ivmanager is filled will null pointers to everything that is related to running the user interface

@JenniferBuehler PR does well: Ivmanager subclasses! @JenniferBuehler PR does poorly: No way to create headlessIVManager in graspit

The main problem I am having in merging these two approaches is that I don't know where to put the Viewer: 1) Do we want other classes that call getViewer() to have pointers to an InteractiveIVManager subclass? or 2) do we want the base class IVManager to have virtual functions for render(), or getViewer() , and just have them essentially no-op in the headless subclass?

Should we even expose getViewer() outside of ivmgr if there isn't always going to be one?

JenniferBuehler commented 8 years ago

Yes it would be nice to combine the two approaches. I'm with you that the best would probably be to go through all classes that use IVmgr and make sure they absolutely need to access the interactive functions. Ideally, only those classes which have any concept of interactivity (eg. the ui classes) use InteractiveIVManager functions, leaving all other classes to use only non-interactive functions which can be implemented in the HeadlessIVManager (eg to get world, etc). Classes which need the InteractiveIVManager (eg because they render things) could either only accept references to the interactive one in the first place, or dynamic_cast it if it's needed (we could also have a function IVManager::isInteractive() to check for this). But not sure how straight-forward it would be to separate the concepts. I can look at it tomorrow. Will also check out your headless branch to see how it works with the command line parameter.

mateiciocarlie commented 8 years ago

OK, so at this point it seems like perhaps the way to go is to have a new entry point uber-class that:

It seems to me as well that almost anybody calling either the GUI or the ivmgr does it just to get to the world. All those entities will use this new class instead.

In this context, the ivmgr is not just a plugin, as it has special status: other plugins might need it (to do user interaction). The ivmgr is a service offered by GraspIt to the world. However, we would not have a a headless ivmgr: if one does not exist, this uber-class simlpy returns NULL when somebody requests the ivmgr.

I will be mostly offline the next couple of days, but thanks again @jvarley and @JenniferBuehler for pursuing this.

jvarley commented 8 years ago

uberGraspit.cpp?

Let me know what you guys think of: https://github.com/graspit-simulator/graspit/pull/51

JenniferBuehler commented 8 years ago

I will write my comments in your #51 since it seems to be a replacement for this PR. Looks like a good solution to start with.

jvarley commented 8 years ago

I believe we can close this now?

JenniferBuehler commented 8 years ago

Yes, this has been solved alternatively in #51.

On Fri, Jul 1, 2016 at 4:52 PM, jvarley notifications@github.com wrote:

I believe we can close this now?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/graspit-simulator/graspit/pull/38#issuecomment-229967427, or mute the thread https://github.com/notifications/unsubscribe/AMP2ina6jCz2N0tyjlf_HVw_AUxZHpWXks5qRSmagaJpZM4IhR8w .