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

Get rendering into SceneGraph #9540

Closed
35 tasks done
SeanCurtis-TRI opened this issue Sep 27, 2018 · 27 comments
Closed
35 tasks done

Get rendering into SceneGraph #9540

SeanCurtis-TRI opened this issue Sep 27, 2018 · 27 comments

Comments

@SeanCurtis-TRI
Copy link
Contributor

SeanCurtis-TRI commented Sep 27, 2018

Rendering should ultimately belong in SceneGraph (it is, after all, an operation on geometry -- the query is: given camera properties, what is visible?) Currently, it is embodied in the RgbdRenderer in drake/sensors. This issue tracks the work to move it from there into SceneGraph (changing things to conform to the more general SceneGraph paradigm).

The following list of anticipated PRs will track the progress for moving rendering into SceneGraph (the list may evolve if it turns out any given PR is too large or too small):

/cc @kunimatsu-tri , @thduynguyen , @DamrongGuoy, @sherm1

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Sep 27, 2018

I'm not sure exactly what physical design is proposed here, but I'd like to push us toward "pay for only what you need". Would the VTK, OSPRay be dependencies of SceneGraph, or would SceneGraph merely provide enough outputs or data to allow the user to attach a renderer, in cases where its needed? Since SceneGraph is such a core component of almost any simulation moving forward, I would prefer that it never hard-depend on any rendering engine, since they are finicky.

@SeanCurtis-TRI
Copy link
Contributor Author

Short answer

This branch contains all but the last step above (https://github.com/SeanCurtis-TRI/drake/tree/gw_renderer) -- although not organized in the ordering indicated above. I'm currently decomposing it.

Long answer

So, the RgbdRender would die. It depends on RBT. A renderer has to contain the geometry registered by arbitrary sources. So, the equivalent to the renderer would move into the geometry folder. My design includes the following features:

  • SceneGraph would ultimately own three different renderers (which I term low, medium, and high fidelity, corresponding to simple opengl, PBR OpenGL, and path tracing).
  • RgbdCamera basically contains enough to define its intrinsic and extrinsic matrices and provides those values to the geometry::QueryObject and gets an image back (which it puts on its output port).
  • Arbitrary cameras can be configured to use arbitrary rendering fidelity.
  • Only those geometries that have been given the "perception" role would be registered with the various rendering engines.
  • One rendering engine is shared across all cameras.

Speculative features

  • Add which rendering engines any particular SceneGraph should support an construction (i.e., if you know you're only ever doing one type of fidelity, you'd like to not have to pay any runtime cost for the other, unused renderers.)

@jwnimmer-tri
Copy link
Collaborator

I think we'll have to explore some tweaks before we get to the rendering engine integration. I don't think its acceptable for anyone who wants to use, say, MultibodyPlant, to have to always link in VTK or OSPRay. In their own ecosystem, they may have incompatible version of those dependencies. If they were trying to use vision simulation, perhaps we can insist that they consume our VTK. If they are just doing some collision queries for a dynamics simulation, though, they should not be tainted by spurious physical dependencies on a rendering engine. Hopefully there is a way to carve out the rendering-engine dependencies into add-on classes, even if there is still a 1:1 relation from an engine to a SceneGraph and they are highly coupled with internal APIs.

@SeanCurtis-TRI
Copy link
Contributor Author

I'm definitely sympathetic with that concern and am fully in favor of resolving it.

Would I be correct in my belief that the current rendering architecture has the same problem? It seems so to me.

@jwnimmer-tri
Copy link
Collaborator

On quick glance, RgbdCamera has a hard-dep on VTK, but it's decoupled enough that we could sever it if we wanted to without too much work. I don't think it spuriously bring in OSPRay though? In any case, geometry uses other than vision simulation don't bring in VTK spuriously.

@SeanCurtis-TRI
Copy link
Contributor Author

The current branch has a similar layer of abstraction. SceneGraph only knows about the RenderEngine interface (at least in its header). And all of the nasty vtk (and other future headers) are buried in the .cc files. So, it has a similar severability.

Off the top of my head, the only obvious solution I have is a constructor that takes rendering flags combined with the use of dlload. Is this what you have in mind?

@SeanCurtis-TRI
Copy link
Contributor Author

One last question for you @jwnimmer-tri.

I'd like to parallelize some of this stuff. For example, the introduction of the SceneGraph-compatible version of the vtk renderer in the geometry space (removal of RBT dependency and generalization of rendering). However, it will propagate the hard dependencies. Do you feel strongly enough that we defer that task listed above until we have a chance to address dependency issues?

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Sep 27, 2018

Off the top of my head, the only obvious solution I have is a constructor that takes rendering flags combined with the use of dlload. Is this what you have in mind?

Can I ask why you suggest dload? It seems that there's enough of a runtime abstraction that sawing off VTK / OSPRay / Godot / whatever should be simple enough?

What if you just had an option to take in a RenderEngine* of nullptr? That way you don't pay the cost for using it if you don't care?

EDIT: I see that you have fidelity flags, and a low_render_engine_ field (and I'd assume other fields to correspond to other levels).

However, that seems a bit odd. Why not have some sort of open association via strings, such that users can choose what their renderers are? (e.g. std::map<string, RenderEngine>?)
This could permit easier tuning, perhaps?

And I haven't seen how these renderers would be specified yet (from briefly looking at the code); can I ask how you envision this happening?

@SeanCurtis-TRI
Copy link
Contributor Author

Lots of different points:

  1. why dload? This is predicated upon my understanding that unless I do dload, upon loading the drake library, the OS will look for the VTK library and throw a fit if it doesn't find what it's looking for. Whereas, by using dlload, it will only have a fit if we've actively chosen to load the library.

  2. Fidelity flags and engines. Yep, today there's just the one, but in my mind we'll have three (two in the near future and hopefully three in the not-too-distant future.)

  3. I don't understand your "open association" proposal. I need me you to walk me through the problem that you're solving so I have context to appreciate the solution you're proposing. Unfortunately, being "a bit odd" doesn't give me enough to work with. :)

  4. Specifying renderers -- there are two separate issues:

  • The question of which renderers are available at all in SceneGraph is does not appear anywhere in the branch; that's a new discussion in this issue. The plan in the branch is that all renderers are available all the time (although, I had already sketched out what it would take if you didn't want to pay the run-time memory cost of populating more than a single renderer -- simple and not a problem.)
  • The question of how a camera picks a renderer is in the render methods. The CameraProperties takes the fidelity flag (so when you instantiate an RGBD camera, in addition to the normal camera things, you also say which renderer you want it to use.)

@RussTedrake
Copy link
Contributor

i'll throw in a vote for bringing it in more quickly and breaking the linking dependencies in a follow-on, simply for the sake of expediency.

@jwnimmer-tri
Copy link
Collaborator

... breaking the linking dependencies in a follow-on ...

I am all for moving quickly, but if we are going to offer an "image_output_port" on SceneGraph, we need to be sure that it is feasible to update its implementation in the future to avoid the dependencies. (Like, I will do it immediately after image_output_port gets added to master.) We don't want to put ourselves in the position of having to remove the image_output_port from SceneGraph later on (and move it to a different System) as the only path to resolving the dependency issue.

Also, if MultibodyPlant starts to depend on VTK8, we will no longer be able to use MultibodyPlant in many places within Anzu. Some parts of Anzu use third-party libraries that depend on VTK6.

@jwnimmer-tri
Copy link
Collaborator

Why dload? This is predicated upon my understanding that unless I do dload, upon loading the drake library, the OS will look for the VTK library and throw a fit if it doesn't find what it's looking for. Whereas, by using dlload, it will only have a fit if we've actively chosen to load the library.

That libdrake.so will always depend on VTK is on par with the current situation. That is not my complaint. My complaint is users in the bazel_drake_external workflow (which includes TRI Anzu, as well as several research labs) who build Drake from source, need to be able to avoid the dependency. It's all about the deps = [] lines in BUILD files. If I only deps on SceneGraph, I don't want VTK to be linked in. If I then also deps on VtkRenderingEngine and do SceneGraph::AddRenderer(make_unique<VtkRenderingEngine>) I've declared my desire for VTK and thus I depend on it and can use it. Different of my applications of MultibodyPlant / SceneGraph can make different build-time and run-time choices.

Separately, I also still in general support libdrake.so being sliced into smaller pieces to curate each one's dependencies, but that is not directly related here. If we have the logical class design and physical component factoring right (as proven out by drake_bazel_external use case), we can still factor libdrake.so installation at a future point.

@SeanCurtis-TRI
Copy link
Contributor Author

Ok, I think my mental model is growing to encompass what you have in mind.

  1. If I'm a "bazel_drake_external" workflow user, I can set up my project to reference specific bazel targets. The only thing my binary will depend on (in the sense of what the OS is going to look for upon loading) is the exact tree of dependencies that my declared dependencies imply. Therefore, if the scene_graph target, directly or indirectly, has a dependency on VTK, the external user is stuck.
  2. However, if SceneGraph has a RenderEngine dependency (which is just an interface). And each render engine implementation is its own target, the end user can elect to also be dependent on any of the engine implementations they like, a la carte.

To summarize, the targeted workflow has the flavor that it allows end users to create a custom slice of all of Drake to be the libdrake.so and we want to give them a reasonably fine-grain ability to slice.

Do I have that basically right?

@jwnimmer-tri
Copy link
Collaborator

Yes, that's the gist of it.

The only detail to nitpick is that bazel_drake_external users don't use anything akin to libdrake.so -- they are (mostly) just linking all of the *.o files directly into their applications (drake/geometry/scene_graph.o, etc.). (Yes, that means the linker is fed a long list of files.)

@RussTedrake
Copy link
Contributor

I also think that factoring libdrake.so has to be in our roadmap soon. Looks like we have #7294 to track it.

@SeanCurtis-TRI
Copy link
Contributor Author

RenderEngine abstraction got stopped a few weeks ago -- requests for a design change on how RenderLabels are managed led to a rearchitecture that was interrupted by other issues. The RenderLabel rearchitecture is just about ready for a PR, so the chain will continue in the next day or so.

@SeanCurtis-TRI
Copy link
Contributor Author

Adding RenderLabel into the abstract RenderEngine currently under review.

@SeanCurtis-TRI
Copy link
Contributor Author

Expect RenderLabel/RenderEngine (#11331) will merge today; RenderEngineVtk will enter review today.

@SeanCurtis-TRI
Copy link
Contributor Author

RenderEngineVtk (1/2) PR 11477 under slow review. Initial reviewer proved to be unavailable; it was reassigned. This PR doesn't have unit tests; they'll come in the immediate follow up PR.

@SeanCurtis-TRI
Copy link
Contributor Author

Missing render engine feature added (#11545). Began 4-PR chain to get rendering exposed all the way through the SceneGraph/QueryObject API. Updated tasks accordingly.

@jwnimmer-tri
Copy link
Collaborator

Where is the current WIP branch for this feature? It doesn't seem to be gw_renderer anymore. Per the physical design discussion upthread, I would like to see how "Add full rendering API all the way through SceneGraph and QueryObject" affects the BUILD file dependencies.

@SeanCurtis-TRI
Copy link
Contributor Author

SeanCurtis-TRI commented May 29, 2019

Yeah -- sorry about that. That was the "raw" branch. This is the one that's been maintained and is being curated into commits: https://github.com/SeanCurtis-TRI/drake/tree/gw_renderer_remaining. Specifically, commit cd3f06b. (Edit: Sorry, the specific commit, while representative, is not reliable; I'm constantly rebasing. However, the commit message should be pretty obvious. the next three PRs I submit will directly expose rendering through the SG/QO interface. I think you'll find it benign.)

@jwnimmer-tri
Copy link
Collaborator

I think you'll find it benign.

Yes indeed, I confirm it avoids having MultibodyPlant depend on VTK.

It adds more instances of package dependency cycles (geometry vs geometry/render vs systems/sensors vs etc.), but those were already there. I am holding off identifying and proposing how to fix those until #10675 is resolved.

@SeanCurtis-TRI
Copy link
Contributor Author

Sounds good.

@SeanCurtis-TRI
Copy link
Contributor Author

The 4-PR chain to integrate the public API is in progress. FIrst merged, second under platform review. Two subsequent PRs are waiting in the wings.

@SeanCurtis-TRI
Copy link
Contributor Author

The RgbdSensor PR is stalled to incorporate updates in terminology started in #11337.

@SeanCurtis-TRI
Copy link
Contributor Author

The last box has been checked. Finally closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants