Remove Renderer #1477
Remove Renderer #1477
Conversation
Tests shouldn't have dependencies on other modules. We also weren't actually passing `visualTest = True` anywhere anyway, so the code was unused.
We're about to remove the Renderer class and associated components, and these tests use them. This is the only part of this whole remove-the-renderer operation I feel bad about. There is some decent test coverage here, some of which is still relevent to the _new_ GLRenderer in Gaffer, which doesn't have as good coverage. On the bright side, it wasn't being run on CI anyway, and there's a good chance that any significant future overhaul of hardware rendering in Gaffer will mean a move away from OpenGL.
This was used to render via the ancient `IECoreScene::Renderer` class, which has been superseded by a new Renderer class in GafferScene. The new Renderer is just passed objects directly, and doesn't have a method for each object type.
These are no longer used now that we're removed `Renderable::render()`.
This was superseded by Gaffer's own Renderer class way back in 2016. Removing it paves the way for Gaffer's version making its way back into Cortex.
This is unused now we've removed Renderer. In Gaffer's representation, the transform is held outside the CoordinateSystem anyway, and having an internal one was confusing and prone to error.
This serves no purpose now that Renderer is removed. Gaffer has an entirely different representation for options.
I don't see how this can be useful any more, since none of our renderer backends support PatchMeshes. We're also moving away from Ops in favour of Algo.
It returns Group objects, which are not useable or renderable in Gaffer. This could probably be salvaged by returning a single PointsPrimitive with per-point information to identify the station, but someone with skin in that game would need to step up to do it.
This removes the last use of the Group class.
These are no longer useful for rendering because the Renderer class has gone. And they've not useful in Gaffer because it represents grouping, transform and attributes using a different mechanism.
This has been obsolete for a long time - in Gaffer we use ShaderNetwork to represent lights.
Gaffer's new Renderer API doesn't have a world block, or a concept of pre/post world in the old RenderMan API sense. The whole point of modern rendering APIs is to allow anything to be edited at any time in any thread, and stateful APIs impede that. So it doesn't make sense to categorise certain renderables under a PreWorldRenderabel moniker.
This is yet another class that doesn't play any part in Gaffer's scene representation.
Like PreWorldRenderable, this doesn't make a distinction worth having in Gaffer's scene representation. In fact, CoordinateSystems are treated as objects in Gaffer, and only Shaders are treated as attribute state.
There was a problem hiding this comment.
Removals look good to me, just two questions inline about some potential leftover detritus. I'll leave it to @ivanimanishi for the IE perspective and final approval.
|
Would be good to get your 👀 on this @ivanimanishi. |
|
Yeah. I was planning to do it today. |
|
Thanks - that would be great if you can find the time... |
There was a problem hiding this comment.
I think we can try to handle the removal of the Renderable class here at IE, by replacing it by something else where it's being used (including in the modules that are now internal to IE, like IECoreMaya). Worst case scenario we could just reimplement it internally, though we might reach out to you John later to get some pointers.
I am more concerned about all the removals from IECoreScene that could be written to an scc file. Group in particular because we seem to be actively creating those types when writing files from houdini. Even if there is a different way to represent groups now, I don't want to risk being unable to open older files, so I'm against any changes that could have that result.
Since I'm not really familiar with the code being changed, there is a good chance that I got something wrong, and the deprecations in this PR would not affect anything that could be written to a scene file...
Changes
Outdated
| @@ -15,7 +15,7 @@ Breaking Changes | |||
| - Renderable : Removed `render()` method. | |||
| - CoordinateSystem : Removed transform. | |||
| - Font : Removed `meshGroup()` method. | |||
| - Options : Removed. | |||
| - Options, Group, Transform, MatrixTransform, MatrixMotionTransform, AttributeState : Removed. | |||
There was a problem hiding this comment.
What happens if we open old sccs that include those types? I would be against a change that breaks previously cached files.
From what I can gather there is a good chance we are using IECoreScene::Group when converting geometries from houdini to our formats.
There was a problem hiding this comment.
For an .scc, you get Exception : Type "Group" is not a registered Object type. when calling readObject() at the specific location containing the group. For a .cob, you get the exception immediately when calling ObjectReader.read().
I'm sure I'm not aware of all the spots it is used, but the main one I know of is in the IECoreMaya scene shape picking code. It would really be better to remove it entirely there, as currently there are two completely different data representations of the same thing : Viewport 2.0 API stuff for the drawing, and IECoreGL::Renderer stuff for the picking. It would be best to move to all Viewport 2.0 stuff if possible. If that's not possible, I would recommend bypassing the (now removed) Renderer class entirely, and just writing the bare minimum code needed to do the picking, using
I understand that concern, but I'm confident that there is no native Gaffer functionality that allows you to view, edit or render such groups. For reference, here is the Gaffer branch with the changes to accompany this PR : https://github.com/johnhaddon/gaffer/tree/cortexRendererRemoval. From memory, ToMayaGroupConverter might be one place that would consume groups from Houdini, so it's certainly possible that the IE pipeline might be affected. Since we're in the process of separating Gaffer and IE concerns here, I would propose that Group is resurrected in an internal IE module, and lives there for as long as it continues to be needed. |
As discussed on #1472, this removes the Renderer class in favour of the new equivalent that has been incubating in Gaffer for some time. I've taken this to its logical conclusion by also removing a bunch classes which only made sense in conjunction with the old class. To remove Group I also had to remove CurveExtrudeOp and IDXReader, but I'd be surprised if those are in use anywhere - they're certainly not in Gaffer.
@ivanimanishi, I don't know if IE has unexpected dependencies on any of these so you'll probably want to take a close look. I've split it all up into a series of logical commits so we can pick and choose to some extent if necessary.