Light linking #1985

mattigruener · 2017-02-25T02:39:20Z

Hey John :)

Ok, so this is one of those PRs that will start a discussion about how to actually do this.

I have a simple, but working prototype for the light linking internals that I would like to get some early feedback on in case that you don't like any of this at all. It will need some cleaning up in some areas, but I guess for the most part this is how I could imagine this being implemented. Can I pick your brain on what you'd like to have changed? I am sure I'm violating some conventions again.

Building a UI for this will be a pain at some point too, but I'm ignoring that for now. I will send you the example scene that I've been using for this via eMail, because it contains some IE nodes (should have stripped them out, but I gotta run).

Thanks for your help :)
Matti.

johnhaddon

Good stuff Matti!

I've made a lot of small comments on stuff that you might be about to tidy up anyway, but I made them just in case you weren't aware of the odd one. I've also made a couple of bigger comments about how we organise things and a couple of potential bugs, but I don't think there's too much work there.

I suggest we either rename SetExpression.h to SetAlgo.h or just move evaluateSetExpression() to SceneAlgo.h. As a general rule, free functions in Gaffer are organised into an Algo file of some sort.

The naming for the attributes is a bit of a tricky one isn't it? Typically Gaffer just passes around dumb attribute values which are passed directly to the renderer backends without further processing, so the the renderer backends provides the semantics for the attributes. Where this hasn't been the case so far, and the attribute instead drives some aspect of Gaffer's scene output process, we've prefixed the attribute with "gaffer:" to emphasise that it controls Gaffer rather than the renderer - I'm not sure whether or not that makes sense here though. Do you think Gaffer would ever see a "lightsToBeLinked" attribute directly? Maybe if one had been written into a scene cache? Would that mean we need the two types to coexist in Gaffer somehow? What do you think to only having one attribute name (the one defined for the renderer), but have Gaffer add the additional smarts of also allowing it to specify a set expression, converting it the StringVectorData at rendertime as you've implemented already? Any other thoughts @danieldresser, @andrewkaufman?

Presumably we'll be adding whatever attribute name we settle on into the StandardAttributes node?

Do you intend to add any more features to the expression language to this PR? I was thinking parenthesis for grouping might be nice, and possibly also treating a "space separated list of sets" the same way as a "pipe | separated | list | of | sets" to provide simple functionality for folks with expression blindness. Perhaps also an "/explicitly/named/light" syntax might be handy? No need for that now if you'd rather keep it simple to start with though.

No need to add it for this PR, but at some point I'll need a hashSetExpression() method as well. Presumably it'll be fairly straightforward to implement that using the same parser and AST, but a different visitor?

It's great that you've got an end-to-end prototype together to prove the concept first, but I would definitely suggest backtracking a bit now and building it up with unit tests for each logical stage. Maybe start with PathMatcher.intersection(), including some tests that would expose the bug I mentioned? Then add tests for the set expression, and finally the renderer output? How does that sound?

johnhaddon · 2017-02-27T17:59:13Z

include/GafferScene/SetExpression.h

+namespace SetExpression
+{
+
+GafferScene::ConstPathMatcherDataPtr evaluateSetExpression( const std::string setExpression, const GafferScene::ScenePlug* );


Should be const std::string &, so we don't need a copy. I think we should probably return PathMatcher rather than PathMatcherData as well, since the data wrapper doesn't add much value here (PathMatchers are already cheap to copy).

johnhaddon · 2017-02-27T17:59:52Z

include/GafferScene/SetExpression.h

@@ -0,0 +1,58 @@
+//////////////////////////////////////////////////////////////////////////
+//
+//  Copyright (c) 2012, John Haddon. All rights reserved.


Nothing of mine here...

johnhaddon · 2017-02-27T18:00:15Z

include/GafferScene/SetExpression.h

+
+GafferScene::ConstPathMatcherDataPtr evaluateSetExpression( const std::string setExpression, const GafferScene::ScenePlug* );
+
+} // namsepace setExpression


Spelling, capitalisation

johnhaddon · 2017-02-27T18:00:27Z

include/GafferScene/SetExpression.h

+#include "GafferScene/PathMatcher.h"
+#include "GafferScene/ScenePlug.h"
+
+namespace Gaffer


GafferScene

johnhaddon · 2017-02-27T18:03:46Z

src/GafferScene/SetExpression.cpp

+#include <vector>
+#include <string>
+
+namespace qi = boost::spirit::qi;


Duplication

johnhaddon · 2017-02-27T18:17:34Z

src/GafferScene/SetExpression.cpp

+	}
+	else
+	{
+		printf("No! No! No! You're doing it wrong! I'll give you something equally as wrong!");


Presumably we need to throw something more descriptive here? Or should we be trying to catch the problem at the parsing stage?

I guess we wouldn't even get to this point, because the parser wouldn't recognize any other operator and bail on us. That needs a proper error btw - potentially with an indication of how far into the SetExpression parsing succeed?

Could not fully parse SetExpression "(mySetA | mySetB) % mySetC" ^

The current message is definitely nothing more than a placeholder. I hadn't considered that we prbly don't even have to handle that case in the AstEvaluator...

Ah, I see. Maybe the real issue then is that we're using char in BinaryOp, so our AST allows greater freedom than our parser does? How about we add an enum Op { Intersection, Union } to BinaryOp, and store one of those instead of a char? That way we can use a switch instead of if...else..., and the compiler will warn for any unhandled cases if we add further operations in the future.

johnhaddon · 2017-02-27T18:22:55Z

src/GafferArnold/IECoreArnoldPreview/Renderer.cpp

+
+				if( m_linkedLights )
+				{
+					const std::vector<std::string> &lightNamesWritable = m_linkedLights->readable();


The Writable part of the name seems misleading.

Ah, yeah - I used ->writable() before and forgot to fix the variable name when I fixed the method call.

johnhaddon · 2017-02-27T18:25:41Z

src/GafferArnold/IECoreArnoldPreview/Renderer.cpp

+					int i = 0;
+					for ( IECore::StringVectorData::ValueType::const_iterator it = m_linkedLights->readable().begin(); it != m_linkedLights->readable().end(); ++it, ++i )
+					{
+						// TODO: Can we avoid prefixing the path here?


I think this is unavoidable. The issue is that Arnold lights can consist of not only the light node, but also a polymesh node in the case of the mesh_light. So, we always name geometric nodes with /the/usual/name and then prefix lights with light:/the/usual/name to allow the two to coexist.

I don't know how you feel about that kind of stuff, but I had thought we could create some kind of utility method that does the prefixing, which we would then call from both https://github.com/GafferHQ/gaffer/blob/master/src/GafferArnold/IECoreArnoldPreview/Renderer.cpp#L1362 and from here. I understand the need for prefixing - I just thought it's a little ugly to duplicate the (admittedly small) logic for prefixing? If we ever need to change the prefix, we'd have to do it twice. Maybe I'm overly concerned about something that will never come up :P

I understand the need for prefixing - I just thought it's a little ugly to duplicate the (admittedly small) logic for prefixing? I don't know how you feel about that kind of stuff, but I had thought we could create some kind of utility method that does the prefixing.

Oh, I see. Personally I think I'd probably just leave it...

johnhaddon · 2017-02-27T18:37:38Z

src/GafferScene/Preview/RendererAlgo.cpp

@@ -512,6 +513,21 @@ struct LocationOutput
 				updatedAttributes->members()[g_setsAttributeName] = boost::const_pointer_cast<InternedStringVectorData>( setsAttribute );
 			}

+			// TODO: clean up
+			CompoundObject::ObjectMap::const_iterator it = updatedAttributes->members().find( "linkedLightsSet" ); // TODO: naming needs to be determined


The purpose of this function is to update m_attributes using the additional attributes computes at line 495 and stored in the attributes variable. So I think the find() here should be performed on attributes and not updatedAttributes.

I have fixed it in one of the latest commits. I guess my reasoning was that we are pretty much done with those attributes after we copied over everything in https://github.com/GafferHQ/gaffer/blob/master/src/GafferScene/Preview/RendererAlgo.cpp#L507. We could just operate on the new data after that, I guess, but the latest code will use attributes now :)

But updatedAttributes is initialised from m_attributes, which is inherited from the parent. So if you do the find() in updatedAttributes you would be redoing the work for every single descendant of the location that actually had the attribute assigned.

Ah, makes sense. Thanks! :)

johnhaddon · 2017-02-27T18:40:25Z

src/GafferScene/Preview/RendererAlgo.cpp

+
+				if( StringDataPtr lightsExpressionData = IECore::runTimeCast<StringData>( it->second ) )
+				{
+					GafferScene::ConstPathMatcherDataPtr linkedlightsSet = SetExpression::evaluateSetExpression( lightsExpressionData->readable(), scene );


It seems that if we have many locations all with the same set expression applied, we'll be computing the same expression multiple times, once for each location. I think we're gonna need some sort of caching for that instead. The RenderSets class is already doing something similar for storing object sets, so perhaps it should also be the class to provide the cache for light link sets?

I have added a commit that takes a first stab at the caching. I'm not sure if that's what you had in mind and I'm particularly unsure of how much you dislike my use of mutable in this case.

mattigruener · 2017-02-27T19:27:32Z

Hey John,

thanks for the feedback. I'll look into addressing most of it today. Just as an additional question that's also related to the tests: How do you feel about adding a ScenePlug::evaluateSetExpression() as an addition to ScenePlug::set() and ScenePlug::setNames()? I was talking to Dan about it and we both felt that doing it that way could be useful as a python binding? A ScenePlug is necessary anyway for the SetExpression to make any sense. The test for the second stage (evaluating the expression) could be done very easily that way.

I'll comment on the rest of your feedback as well - just wanted to get this out there while you're still around maybe :)

Matti.

mattigruener · 2017-02-27T23:31:38Z

Do you think Gaffer would ever see a "lightsToBeLinked" attribute directly?

Are we planning on extending the lighting preview in the viewport in some way? Fancier OpenGL stuff at some point? Would we want that to respect light linking? Ha, answering your questions with new ones is probably bad style, but I guess that would be a case in which Gaffer would have to be aware of how lights and geometries interact? Maybe that would be handled in a renderer backend the same way we do it for Arnold, though...
The only other reason, that I can think of, for evaluating the expression earlier and adding the "lightsToBeLinked" attribute in the sceneGraph would be if we fail to design a good UI that operates on the SetExpressions? Passing around an explicit list of lights would probably simplify how we build the UI for light linking?

Does that make sense?

mattigruener · 2017-02-28T02:56:27Z

I hope you can see all the comments I made during the day - github shows all the respective parent comments as outdated, but I guess you received an eMail about them...

johnhaddon · 2017-02-28T15:06:54Z

I hope you can see all the comments I made during the day - github shows all the respective parent comments as outdated, but I guess you received an eMail about them...

I can, by popping open the "Show outdated" sections. I've replied to a bunch of them as well, so hopefully you can see 'em by popping in there too...

How do you feel about adding a ScenePlug::evaluateSetExpression() as an addition to ScenePlug::set() and ScenePlug::setNames()? A ScenePlug is necessary anyway for the SetExpression to make any sense.

The way I see it, set expressions are something that operate using a ScenePlug, but are not an intrinsic part of the ScenePlug itself. There are all sorts of operations that take a ScenePlug, but if they were all implemented as members then we'd end up with a very cluttered API. I've actually considered going the other way, and moving all the helper methods from ScenePlug into SceneAlgo.

I found this article fairly informative when thinking about this stuff. Following the "one class, one responsibility" rule I would say that the responsibility of ScenePlug is to represent a scene graph in Gaffer's plugs/contexts framework, and therefore the existing members (plug accessors, and helpers to compute those plugs in a specific context) make sense. But any higher level operations belong elsewhere.

I was talking to Dan about it and we both felt that doing it that way could be useful as a python binding? The test for the second stage (evaluating the expression) could be done very easily that way.

Absolutely we need it as a python binding, and we need tests. But it can just be bound as a non-member function, like the rest of SceneAlgo.

I'll comment on the rest of your feedback as well - just wanted to get this out there while you're still around maybe :)

Sorry - I wasn't around. I wasn't officially working yesterday, but just found a bit of time to do a first pass review because I didn't want you to twiddle your thumbs...

Are we planning on extending the lighting preview in the viewport in some way? Fancier OpenGL stuff at some point? Would we want that to respect light linking? Ha, answering your questions with new ones is probably bad style, but I guess that would be a case in which Gaffer would have to be aware of how lights and geometries interact? Maybe that would be handled in a renderer backend the same way we do it for Arnold, though...

Good question - that's another thing to inform our decision. I'm still hoping to make IPR so good that noone cares about GL, but I think it'd be reasonable for us to design something such that OpenGL could respect light links if we wanted. If we did that, then I think what we'd do is reimplement the OpenGL translation with an OpenGLRenderer class as you suggested. That's actually how it was in the first iteration of Gaffer, but the original Cortex renderer API wasn't up to the job when it came to describing edits so the viewer ended up with custom code. The new API we use with Arnold is probably good enough though, so we could implement an OpenGLRenderer subclass using that, and then drive the GL viewer with the same mechanism used by the GafferScene::Preview::InteractiveRender node.

Long story short, I don't think we need to worry about GL now, because the work we do to support light linking in the InteractiveRender (I presume you're planning on adding this?) will be applicable to GL in the future.

The only other reason, that I can think of, for evaluating the expression earlier and adding the "lightsToBeLinked" attribute in the sceneGraph would be if we fail to design a good UI that operates on the SetExpressions? Passing around an explicit list of lights would probably simplify how we build the UI for light linking? Does that make sense?

Let's call the two potential attribute forms the "expression" and the "list" for ease of explanation. I don't think there's any reason to prefer the list over the expression in Gaffer itself - after all, we can always get one from the other. The thing I'm trying to get at is that the expression attribute will inherently be a gaffer feature, requiring Gaffer's output logic to be used to get the intended render. Whereas the Renderer API is theoretically intended to be used by anyone who wants to talk to a renderer (a procedural for instance, or someone writing a Gaffer alternative). In that scenario only the list attribute makes sense, which means there may be workflows where cache files store the list attribute. Which means that when loaded in Gaffer we might see the list attribute alongside the expression attribute, with all the attendant ambiguities that brings.

Actually, I've just realised that if we add the "/name/a/light/directly/in/place/of/a/set" syntax I proposed to the expression language, then the expression is a just a superset of the list.

I think I'm leaning towards making the list and the expression attribute be the very same thing, with the same name ("linkedLights" or "lightLinks"?) in the Renderer API and StandardAttributes node. So it can be used naively elsewhere, but has added bells and whistles when used in Gaffer.

This brings me to another thought. It's going to be a pain to implement the light link outputs for the InteractiveRender node as well as the Render node, and the caching in the RenderSets version needs work anyway. So maybe it makes sense to instead implement an EvaluateLightLinks node that is used internally by both? And then it could also be used explicitly if we ever wanted to generate a .scc or .abc file with naive lists of lights so they could be used outside Gaffer?

johnhaddon · 2017-02-28T15:13:26Z

src/GafferScene/Preview/RendererAlgo.cpp

-					GafferScene::PathMatcher linkedlightsSet = SetExpression::evaluateSetExpression( lightsExpressionData->readable(), scene );
-					linkedlightsSet.paths( lightNamesWritable );
-					updatedAttributes->members()["lightsToBeLinked"] = lightNames; // TODO: naming needs to be determined
+					IECore::StringVectorDataPtr lightNames = new IECore::StringVectorData();


I should have mentioned that we should probably use the IECore::LRUCache to simplify the caching. This would have taken care of the two big problems in this implementation :

Multiple threads are reading/writing m_lightLinkExpressions at the same time, which will lead to crashes.

Multiple threads may evaluate the same expression because it wasn't in m_lightLinkExpressions when they checked, but was by the time they finished their own evaluation.

Anyway, I've suggested in my main comment that perhaps we should have an EvaluateLightLinks node so we don't need to do anything in the render output layer - let's resolve that discussion before worrying more about the caching.

johnhaddon · 2017-02-28T16:25:44Z

This brings me to another thought. It's going to be a pain to implement the light link outputs for the InteractiveRender node as well as the Render node, and the caching in the RenderSets version needs work anyway. So maybe it makes sense to instead implement an EvaluateLightLinks node that is used internally by both?

On this topic, here's a branch from a while back where I was prototyping a mechanism for registering such adaptors :

https://github.com/johnhaddon/gaffer/tree/renderAdaptors

mattigruener · 2017-02-28T20:36:45Z

Thanks for linking to that article. Good read!

Sorry - I wasn't around. I wasn't officially working yesterday, but just found a bit of time to do a first pass review because I didn't want you to twiddle your thumbs...

Thanks for replying anyway! I have other stuff on my list in case you don't have time to reply :)

Do you intend to add any more features to the expression language to this PR? I was thinking parenthesis for grouping might be nice, and possibly also treating a "space separated list of sets" the same way as a "pipe | separated | list | of | sets" to provide simple functionality for folks with expression blindness.

I am happy to add more to it, but didn't really intend to for this PR. The space separated list is a great idea, though and I'll look into it. Considering the rest of the discussion about using just a single attribute for both list and expression, it sounds like we will need it for this PR anyway?

Can you give me an example regarding the parenthesis for grouping? Something like this

( (A | B) & C ) | D

should already work, I think. Is that what you mean?

Perhaps also an "/explicitly/named/light" syntax might be handy?

Actually, I've just realised that if we add the "/name/a/light/directly/in/place/of/a/set" syntax I proposed to the expression language, then the expression is a just a superset of the list.

Ok, I have a question about that. I just tried and "/name/a/light/directly/in/place/of/a/set" is a perfectly valid name for a set in Gaffer. I guess what we could do in order to distinguish between sets and lights is to interpret a missing set here as a potentially valid light location and create a PathMatcher for it? Maybe only if it starts with a "/"? You didn't really like printing stuff from there anyway - I still feel like it could be confusing to the user, especially if that's how we solve the set vs. light problem. What was your idea for differentiating between a light location and a (admittedly obscure) set name that looks like one? Am I missing something?

I think I'm leaning towards making the list and the expression attribute be the very same thing, with the same name ("linkedLights" or "lightLinks"?) in the Renderer API and StandardAttributes node. So it can be used naively elsewhere, but has added bells and whistles when used in Gaffer.

I like "linkedLights" for a list of lights better, but I don't feel strongly about it either way.

So maybe it makes sense to instead implement an EvaluateLightLinks node that is used internally by both? And then it could also be used explicitly if we ever wanted to generate a .scc or .abc file with naive lists of lights so they could be used outside Gaffer?

Using just one attribute with "added bells and whistles" in Gaffer sounds good to me. I will look into adding the EvaluateLightLinks node and moving the expression eval stuff into it from the renderer back end. Am I right in assuming that we could potentially run in a situation where some .scc brings in a list of lights and we then unnecessarily apply all the bells and whistles only to parse/eval that list into the same list again? Any way to determine that we don't have to (except just doing it once and then rely on hashing)?

Thinking about .scc files bringing in a list that contains all kinds of goofy lights that probably don't even exist in the scene, I realize that I should definitely check for non-existing lights here in order to not clutter that attribute with non-existing nodes. In general, do you feel like it's a useful workflow to store the links in that cache? I'm just wondering if that could lead to weird situations where someone innocently renames a light in his scene and accidentally hits a link that was brought in by a cache only to mess up their lighting completely...

Will also look into your other suggestions and add respective commits.

Thanks :)

mattigruener · 2017-03-01T03:11:35Z

Added a few updates from today.

johnhaddon

Do you intend to add any more features to the expression language to this PR? I was thinking parenthesis for grouping might be nice, and possibly also treating a "space separated list of sets" the same way as a "pipe | separated | list | of | sets" to provide simple functionality for folks with expression blindness.

I am happy to add more to it, but didn't really intend to for this PR. The space separated list is a great idea, though and I'll look into it. Considering the rest of the discussion about using just a single attribute for both list and expression, it sounds like we will need it for this PR anyway?

I don't think we need it in this PR, but it would be nice to have if you get the chance.

Can you give me an example regarding the parenthesis for grouping? Something like this

( (A | B) & C ) | D

should already work, I think. Is that what you mean?

Oh cool - I'd missed that part of the syntax definition. Could you add a test demonstrating it in action? Maybe use ( A | B ) & C vs A | ( B & C ) where C is empty to show the difference? Actually I realise I don't know the operator precedence of | and & here - is it implemented so that & takes precedence over | as in C++?

Perhaps also an "/explicitly/named/light" syntax might be handy?

Ok, I have a question about that. I just tried and "/name/a/light/directly/in/place/of/a/set" is a perfectly valid name for a set in Gaffer. I guess what we could do in order to distinguish between sets and lights is to interpret a missing set here as a potentially valid light location and create a PathMatcher for it? Maybe only if it starts with a "/"? You didn't really like printing stuff from there anyway - I still feel like it could be confusing to the user, especially if that's how we solve the set vs. light problem. What was your idea for differentiating between a light location and a (admittedly obscure) set name that looks like one? Am I missing something?

Good point. I was just assuming we'd embed it straight into the set syntax that something beginning with / was always an explicit path, and never a set name. I think that's better than allowing ambiguity in the parser and then having to resolve it in the visitor. Do you think we can just disallow a / in the name in the Set node, or do you think we have examples of these / prefixed sets in the wild?

Using just one attribute with "added bells and whistles" in Gaffer sounds good to me. I will look into adding the EvaluateLightLinks node and moving the expression eval stuff into it from the renderer back end.

Cool.

Am I right in assuming that we could potentially run in a situation where some .scc brings in a list of lights and we then unnecessarily apply all the bells and whistles only to parse/eval that list into the same list again? Any way to determine that we don't have to (except just doing it once and then rely on hashing)?

Yep, you're right. But maybe we can detect the list at the parsing stage to avoid the entire visitor stage? I think we can leave this kind of optimisation out of this PR though, and apply it only when we need it.

Thinking about .scc files bringing in a list that contains all kinds of goofy lights that probably don't even exist in the scene, I realize that I should definitely check for non-existing lights here in order to not clutter that attribute with non-existing nodes.

Bear in mind that the set expressions are just that - set expressions. Any thinking about lights should be limited to the EvaluateLightLinks (or ExpandLightLinks?) node. I guess this is a fairly straightforward lightSet.intersection( resultOfMyExpression ) though?

In general, do you feel like it's a useful workflow to store the links in that cache? I'm just wondering if that could lead to weird situations where someone innocently renames a light in his scene and accidentally hits a link that was brought in by a cache only to mess up their lighting completely...

I don't feel like it's a workflow we'll go for particularly, but I do think it's useful to be able to cache out a dumb representation of a scene in a form independent of Gaffer, to aid potential interoperability with other pipelines. But in Gaffer itself the usual rules will apply of course - if you use sets then Gaffer will keep everything valid for you, and if you use explicit names, then the responsibility is on you.

Will also look into your other suggestions and add respective commits.

Thanks! How do you want to progress with the renderAdaptors thing? Are you planning on just using an ExpandLightLinks node internally to IERenderPass, or shall we also get the renderAdaptors branch cleaned up and ready to go so we can use it for this?

johnhaddon · 2017-03-01T09:52:06Z

src/GafferArnold/IECoreArnoldPreview/Renderer.cpp

 						std::string lightName = "light:" + *(it);
 						AtNode *lightNode = AiNodeLookUpByName( lightName.c_str() );
-						AiArraySetPtr( linkedLightNodes, i, lightNode );
+						if( lightNode )


Does this actually change anything compared to before? We're still leaving holes in the array right? It's just that now they won't be initialised (unless AiArrayAllocate initialises them to NULL for us, in which case nothing has changed)...

johnhaddon · 2017-03-01T09:58:27Z

src/GafferScene/PathMatcher.cpp

@@ -425,10 +425,15 @@ PathMatcher PathMatcher::intersection( const PathMatcher &paths )
 	PathMatcher result = PathMatcher();
 	for( Iterator it = (*this).begin(); it != (*this).end(); ++it )
 	{
-		if( paths.find( *it ) != paths.end() )
+		RawIterator rit = paths.find( *it );
+		if( rit.exactMatch() )


I think this is probably OK given the current implementation of RawIterator, but generally dereferencing an iterator (which is basically what exactMatch() is doing) before checking it points to something (with the comparison to end()) sets alarm bells ringing. Maybe we could reverse these operations so that's it's clearly kosher to the casual observer?

mattigruener · 2017-03-02T02:48:30Z

I have added support for objectNames now, but the visitor still has to do some of the work in the current implementation. Both setNames and objectNames are just stored as strings at the parser stage... I started reworking some of that towards the end of today to use structs for SetName and ObjectName - will continue working on that tmrw.

Cool. Don't stress too much about parsing straight into a vector<InternedString> if it turns into a real pain. It would definitely be great to see, but we can leave some optimisations for later in preference to getting a first version out.

There's a bunch of new tests now that show grouping and the new features. I kinda like that we can effectively define unnamed sets now by doing something like:
A & (/group/group/sphere1 /group/group/sphere42)

Yeah, I like it too.

But I guess that also has the potential to get wild and messy fairly quickly?

For sure. I don't think it's the right way to go about things generally, and I think the fact that explicit names won't be tracked through hierarchy changes will encourage folks to use sets in preference. But I do think it's going to be a very handy feature for quick tests and last minute tweaks.

How do you feel about the current set of features of our little language? Now that we have support for objectNames, it feels like one necessary next step would be adding support for something like this:
bagOfObjects - objectWeDontWant

That was the one obvious omission I could see too. It seems like an easy one to add, since you already have your BinaryOp class and we already have PathMatcher.removePaths( otherPathMatcher ), so could you get it in for this PR do you think?

Dan and me also talked about supporting wildcards for the object names, so that /car/headlights/* actually expands to (/car/headlights/leftLight /car/headlights/rightLight) during evaluation. That could be quite expensive, though. To offset some of that cost, the function that does the evaluation could take a set of objects that define the ones we are interested in at all (all lights, for example)? We would then loop over all those and check if they match the wildcard. Maybe an addition for later? Seemed quite useful to us to support wildcards...

I agree that that might be a natural extension, but let's leave it for now. It takes the implementation to a whole new level of complexity, and it seems like we're otherwise pretty close to having something we can deploy.

Currently it's not even accepting something like

A | B & C

but needs the explicit grouping via (). I realize it's not ideal and I will look into lifting that restriction. Will make things a little more complicated, I guess, but requiring the parenthesis makes the whole thing very lisp'y, doesn't it? ;)

Yeah, I think I'd get confused If A | B & C was a parse error. Does that mean that A | B | C is also a parse error? If so, we definitely need to get that sorted...

Yes, true. General question: Sets are supposed to contain every match explicitly and we don't consider children of objects in the set as part of the set? I'm just asking because I'm wondering if adding /car/headlights to a set should make the children come through as linked lights.

I'm used to light linking being done directly to the light, and not be an inherited/ hierarchical thing. So let's require exact matches for the lights and see how it pans out - the light nodes all provide the ability to put lights directly into multiple sets, so I don't think it'll be hard in practice.

How do you want to progress with the renderAdaptors thing?

Good question - I think it would make sense to get that cleaned up anyway? I am happy to look into it - what did you expect what we'd still have to do on it?

Just took a look over it, and it seems like it's actually pretty close to being done. There's one last commit labelled WIP and seems to be adding support to the legacy 3delight render nodes, but other than that I think it's actually finished. This raises the question of whether or not we're going to support light linking for the 3delight renders too? Personally I feel like we'd be better off saving our effort and hoping to save up some time to add a shiny new NSI or PRMan21 backend to replace the legacy ones. In that case, I could just remove the WIP commit and put the renderAdaptors branch up for review.

I didn't get to adding a test that checks the linked lights on the Arnold side, but I'll look into that tomorrow.

Great! Seems like we're getting close?

mattigruener · 2017-03-02T19:50:45Z

What happened to that latest comment? :P

I didn't even see it this morning, because I am the author of it? Just noticed when I pushed the change that preserves knowledge about setNames and objectNames...

mattigruener · 2017-03-02T22:55:37Z

bagOfObjects - objectWeDontWant

That was the one obvious omission I could see too. It seems like an easy one to add, since you already have your BinaryOp class and we already have PathMatcher.removePaths( otherPathMatcher ), so could you get it in for this PR do you think?

Yes, will do. Should be fairly straight forward although it will need some reshuffling - especially keeping in mind that I have to work on the operator precedence as well.

Does that mean that A | B | C is also a parse error?

Yeah, none of those will currently work. Will look into it. I just added a test that should uncover this problem.

This raises the question of whether or not we're going to support light linking for the 3delight renders too? Personally I feel like we'd be better off saving our effort and hoping to save up some time to add a shiny new NSI or PRMan21 backend to replace the legacy ones. In that case, I could just remove the WIP commit and put the renderAdaptors branch up for review.

I think that would be best. If it should ever come up we can still add it for those nodes, but I'd personally leave it until that happens... I haven't looked at the Appleseed implementation at all, but supporting that would make more sense to me.

Great! Seems like we're getting close?

I think so! With some of the changes that I still need to make I don't know how much more boost::spirit pain they will cause and how long it'll take to sort that out. But that went somewhat smooth so far...

I guess we'll handle a UI for lightlinking in a different PR?

johnhaddon · 2017-03-03T08:56:27Z

What happened to that latest comment? :P

Sorry! Looks like I did my usual muppet thing and opened your comment for editing so I could get the original markdown to cut + paste, and then forgot the cut + paste step. I've tried to find a way of disabling edits but there doesn't seem to be one...

I guess we'll handle a UI for lightlinking in a different PR?

Yes, but I would like to add the linkedLights (lightLinks? maybe a vote?) attribute to the StandardAttributes node, so we have basic support for setting them.

I'll get my renderAdaptors branch up for review today. Since we seem to have a good plan in place now, I think I'll wait until you have all your ducks in a row and your commits squashed before I do a last review (unless you want specific feedback on anything in the meantime). Sounds OK?

mattigruener · 2017-03-03T16:59:18Z

Yes, sounds good :) I'll let you know once I feel like I am done. Thanks!

mattigruener · 2017-03-09T00:34:27Z

src/GafferScene/SetAlgo.cpp

+	qi::rule<Iterator, ExpressionAst(), ascii::space_type> nameOrListOfNames, listOfNames, objectNameOrSetName, element;
+};
+
+void printSyntaxError( const std::string &setExpression, std::string::const_iterator position )


Not sure if you like the way I handle the errors here, @johnhaddon?

WARNING : GafferScene::SetAlgo : Syntax error in indicated part of SetExpression. setA - (/group/group/sphere2 |---------------------|

Adding support for better errors would be a little tricky I guess. We could potentially run a check for unbalanced parantheses here, but meh.

I think a syntax error should throw an exception rather than print a warning on a side channel. The exception will be caught and shown in the node graph on the offending node, which makes it a lot easier to track down.

In terms of formatting, it seems that spirit is only giving the position of the syntax error, but this is showing it as a range from the position to the end of the string? Is it accurate to blame that entire range, taking into account nesting etc? The formatting I'm most used to is what clang is doing these days, and that just points at the position with a ^ :

src/Gaffer/Set.cpp:52:2: error: use of undeclared identifier 'fsd' fsd ^

Would that make sense here?
Would it make sense

I'm using this example again in the following:

WARNING : GafferScene::SetAlgo : Syntax error in indicated part of SetExpression. setA - (/group/group/sphere2 |---------------------|

So yeah, I thought about this one for a bit when I implemented it and in theory I prefer the ^ way of indicating an error. But in this case it's not really saying "Hey, that minus looks off!", but it's saying "Ok, I know that setA is a set name and you're allowed to use a minus here, but only if your second operand matches what I know are valid operands for andNot operations. What you're doing there is weird and it doesn't match what I would expect a valid operand. Something in what you're doing following the set name is fishy".

That's why I felt that simpy pointing to the minus is misleading, because really it's all the rest that spirit has a problem with. Does that make sense?

Maybe a less wordy way of saying this is that spirit indicates up to which point parsing according to the rules in the grammar succeeded. Showing that to the user in the way that clang does it might be misleading?

OK, that makes sense - thanks for explaining. Let's go with what you have then but throw an exception rather than post a message...

mattigruener · 2017-03-09T00:36:29Z

include/GafferScene/SetAlgo.h

+PathMatcher evaluateSetExpression( const std::string &setExpression, const ScenePlug* scene );
+
+IECore::MurmurHash setExpressionHash( const std::string &setExpression, const ScenePlug* scene );
+void hashSetExpression( const std::string &setExpression, const ScenePlug* scene, IECore::MurmurHash &h );


I added both of these because they both seem useful. Do I violate Gaffer conventions by doing that?

We do something similar in ValuePlug where we have IECore::MurmurHash hash() and void hash( IECore::MurmurHash &h ). I think that's worthwhile because for implementing a node the second form is most useful (and gets used a ton) whereas as a client of a node the first form is most useful. Personally I think I'd be tempted to say that set expressions will be used in so few places that just setExpressionHash() is sufficient, but if you want both, maybe we should follow the ValuePlug pattern and have two overloads of setExpressionHash()?

- also fix up tests and bindings

mattigruener · 2017-03-24T02:14:32Z

So, I've been trying to get the tests here and in your other PR to pass throughout the day today and I'll just restart the currently failing job now and then call it a day. Right now, it's failing with what looks like a fairly severe crash on the appleseed side. I created a gist to preserve that output.

johnhaddon · 2017-03-24T16:07:19Z

I mailed Esteban about the GafferAppleseed test and he thinks it's most likely a threading problem with GafferAppleseed. I'm not sure why it's showing up more now, but maybe just because the presence of an adaptor is changing the timings somewhat? He's going to take a look when he gets a chance...

I also learned that the next Appleseed will be the last one before they require C++11. I think it's time to draw a line in the sand ourselves...

mattigruener · 2017-03-24T16:51:34Z

Thanks for looking into it :) C++11 would be great for sure... I guess there's not much left for me to do on this PR for now - do you want me to start cleaning up the history or would you like to keep it for the review?

johnhaddon · 2017-03-24T16:55:37Z

do you want me to start cleaning up the history or would you like to keep it for the review?

Squash it please, let's get this one out there!

…r light linking

also serves as an example for setting up custom adaptors

mattigruener · 2017-03-24T19:07:57Z

I think the history should be pretty clean now.

johnhaddon · 2017-03-25T00:04:00Z

Nice one! Thanks Matti, and thanks @donboie for the (boost) spiritual guidance!

mattigruener · 2017-03-25T00:09:44Z

Woohoo, thanks guys :)

johnhaddon requested changes Feb 27, 2017

View reviewed changes

mattigruener force-pushed the lightLinking branch 3 times, most recently from 5c2893c to 2e2a917 Compare February 28, 2017 03:01

johnhaddon reviewed Feb 28, 2017

View reviewed changes

mattigruener force-pushed the lightLinking branch 2 times, most recently from ae9014d to cff22d1 Compare March 1, 2017 03:09

johnhaddon requested changes Mar 1, 2017

View reviewed changes

mattigruener force-pushed the lightLinking branch 2 times, most recently from 4802ef3 to c13048d Compare March 2, 2017 02:51

mattigruener force-pushed the lightLinking branch from 38d681a to f33df73 Compare March 2, 2017 23:06

johnhaddon mentioned this pull request Mar 3, 2017

Render Adaptors #1994

Merged

mattigruener force-pushed the lightLinking branch 4 times, most recently from a99cadb to 527c169 Compare March 9, 2017 00:29

mattigruener commented Mar 9, 2017

View reviewed changes

mattigruener force-pushed the lightLinking branch from 1f070a9 to c74f938 Compare March 23, 2017 00:43

johnhaddon mentioned this pull request Mar 23, 2017

Render nodes : Don't serialise internal connections #2013

Merged

PathMatcher: add intersection() method

b782ec2

- also fix up tests and bindings

mattigruener force-pushed the lightLinking branch from c74f938 to 25984a3 Compare March 23, 2017 23:19

SetAlgo: introduce SetExpressions

919d2cd

mattigruener force-pushed the lightLinking branch 2 times, most recently from f94f542 to b9114cc Compare March 24, 2017 18:44

Matti Gruener added 2 commits March 24, 2017 11:52

SetAlgoBinding : add bindings for SetAlgo

6bc56b7

SetAlgoTest : add tests for SetAlgo

be2656f

mattigruener force-pushed the lightLinking branch from b9114cc to 9d79729 Compare March 24, 2017 18:54

Matti Gruener added 9 commits March 24, 2017 11:55

StandardAttributes : add attribute for light linking

6f092e7

EvaluateLightLinks : introduce node that evaluates set expressions fo…

e748791

…r light linking

EvaluateLightLinksBinding: add bindings for EvaluateLightLinks node

311e8de

EvaluateLightLinksTest : add test for EvaluateLightLinks node

ad84195

EvaluateLightLinksUI : provide UI

b7eac75

startup/GafferScene : create adaptor for light linking

c77d101

also serves as an example for setting up custom adaptors

IECoreArnoldPreview/Renderer: link list of lights

f718ddb

ArnoldRenderTest: add test for light linking

d6e3e45

ScriptingReference : add documentation on SetExpressions

eec5ea3

mattigruener force-pushed the lightLinking branch from 9d79729 to eec5ea3 Compare March 24, 2017 18:55

johnhaddon merged commit 99c1ab7 into GafferHQ:master Mar 25, 2017

mattigruener deleted the lightLinking branch March 25, 2017 00:09

johnhaddon mentioned this pull request Apr 20, 2017

Add light linking mechanism #150

Closed


		GafferScene::ConstPathMatcherDataPtr evaluateSetExpression( const std::string setExpression, const GafferScene::ScenePlug* );

		} // namsepace setExpression

Light linking #1985

Light linking #1985

Conversation

mattigruener commented Feb 25, 2017

johnhaddon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattigruener commented Feb 27, 2017

mattigruener commented Feb 27, 2017

mattigruener commented Feb 28, 2017

johnhaddon commented Feb 28, 2017

Choose a reason for hiding this comment

johnhaddon commented Feb 28, 2017

mattigruener commented Feb 28, 2017

mattigruener commented Mar 1, 2017

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattigruener commented Mar 2, 2017 • edited by johnhaddon Loading

mattigruener commented Mar 2, 2017

mattigruener commented Mar 2, 2017

johnhaddon commented Mar 3, 2017

mattigruener commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattigruener commented Mar 24, 2017

johnhaddon commented Mar 24, 2017

mattigruener commented Mar 24, 2017

johnhaddon commented Mar 24, 2017

mattigruener commented Mar 24, 2017

johnhaddon commented Mar 25, 2017

mattigruener commented Mar 25, 2017

johnhaddon left a comment •

edited

Loading

mattigruener commented Mar 2, 2017 •

edited by johnhaddon

Loading