Initial implementation of ReplacementVisitor#1726
Conversation
|
I have done a first pass review of the changes. My gut reaction is that it's a lot of code changes for something very specific and that there is likely a simpler, easier to understand and use approach lurking out on the either. |
|
The obvious alternatives are to make any visitor that wants to replace nodes with other nodes replicate all this code in its implementation (which means more code total as soon as there's more than one), or to do something complicated like the some of the For example, with this PR, something like Fundamentally, the approach kind of has to look something like this. There needs to be something resembling the Visitor Pattern to allow operations to happen based on the object type (which is a well established good fit, hence the existing visitor classes existing both in the OSG and VSG), and the sensible place for the code describing how to swap out a child object for another is obviously the class that has the child objects. You could get the same kind of thing if a subclass of the existing |
Pull Request Template
Description
As discussed on a call last year, and alluded to in some PR/issue/discussion comments since, a
vsg::Visitorcan't replace objects in the scenegraph it operates on except by handling it at the level of their parent. If you know the structure of the data being operated on and either the object only makes sense in the context of its parent (e.g. swapping the vertex buffer of aVertexIndexDraw) or can limit the number of parent types to a feasible number (e.g. you know the node you want to swap out will always have aStateGroupas its parent), that's fine, but that's not always the case with real-world data.An example use-case is the
IntersectionOptimizeVisitoron my intersection optimisation branch that swaps out nodes for an optimised class which handles intersections faster (and passes other kinds of traversal onto the original node). At the moment, it explicitly handlesStateGroupbecause that's the parent of the target nodes in the sample data I've got, but to work in the general case, it would need to explicitly handle every VSG node type that can have one or more children. With this PR applied, it can just handle the specific node types it targets and avoid needing to handle every possible parent. It would also make it easier to move the generated intersection proxy nodes higher in the scenegraph when only one node in a subgraph is intersectable.I've implemented a slightly contrived but more obvious and straightforward example for the vsgExamples repo that illustrates how the new visitor type can be used and points out a gotcha I expect users of the class might run into. The PR for that is here: vsg-dev/vsgExamples#392
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
So far, basically just the new vsgExample I made to demo the feature. I've not yet adapted the intersection optimisation branch to use it, as I thought it was a good idea to get more eyes on the implementation before marrying anything to it.
Checklist: