Autodesk / sitoa

Arnold plugin for Softimage
Apache License 2.0
33 stars 16 forks source link

Operators #19

Closed JenusL closed 4 years ago

JenusL commented 5 years ago

Arnold 5.1 added something called Operators. Is this something we need and if so, how could it be implemented?

JenusL commented 5 years ago

So I've been trying out Operators in 3ds Max a bit and it would be nice to have them in SItoA as well. I would like to avoid building a new Graph Editor UI like what was done in MAXtoA. So I propose we do a special node like the closure node. Let's call it operators. This node can only be connected to Output pass shaders, just like AOV-shaders. If that operators node is connected. It will not be treated as a AOV-shader but instead as the Operators tree. The operators themselves will not have any exposed ports so that shaders can't be connected to them. What do you think?

caron commented 5 years ago

Now that SItoA 5.2.0 work is done (congrats to you!) I was looking at these outstanding issues...

I have been thinking about how to best implement Operators and I too don't want to make a custom graph editor. And I think using the Render Tree is the right way to go only because I can't figure a way to get the ICE tree to sit on the Render Options in a way that could be unique per pass.

Regarding, the exposed ports, they would need to be exposed but the port needs to be a custom type that won't allow connections from standard types. We should be able to use the features of the API to block any connections we wan't to protect against.

Here you can see in MtoA why we need Operators to connect in some ways... https://docs.arnoldrenderer.com/display/A5AFMUG/Operators

And one last thing, we might be able to use an un-documented feature of the ShaderDef API. One that @sjannuz doesn't like or at least he didn't like it when he was making SItoA 4.0 and I made the suggestion, now that it is open-source and this area is of limited impact to the overall code base... maybe he will change his mind.

Check this image out... imagine a custom "operators" port on the render pass terminal node instead of here on the material terminal node. image of render tree

I already have some prototyping code for when I proposed this almost years ago. I can push it to my sitoa branch and you can check it out.

JenusL commented 5 years ago

Regarding, the exposed ports, they would need to be exposed but the port needs to be a custom type that won't allow connections from standard types. We should be able to use the features of the API to block any connections we wan't to protect against.

Yeah the operator type ports need to be exposed and of custome type. But I'm talking about other parameters on the operators, like booleans. Those can't be exposed, so that you can't connect a boolean shader to them.

And one last thing, we might be able to use an un-documented feature of the ShaderDef API. One that @sjannuz doesn't like or at least he didn't like it when he was making SItoA 4.0 and I made the suggestion, now that it is open-source and this area is of limited impact to the overall code base... maybe he will change his mind.

What exactly was the concern about that? Would anything break?

sjannuz commented 5 years ago

Just to add some more salt, beware that in a very near future Arnold will let you connect an operator graph locally to procedural objects such as alembic and procedural itself.

JenusL commented 5 years ago

Thanks for the info! We should take that into consideration.

caron commented 5 years ago

What exactly was the concern about that? Would anything break?

Well it's more about the unknowns. If we needed a bug fix to make it work properly, we wouldn't get it any longer. Imagine if our custom port didn't get pulled the same way as standard ports during IPR. Unfortunately, the Softimage devs never pushed/promoted this feature of the API so it never got put through the ringer during active development. Bummer too, cause I really liked this feature...it would have allowed for renderer plugins shader/materials to coexist better.

In the case of putting this custom port on the pass, I tried it really quick... custom op port image Now you would need to hack the RenderPass.spdl to get an ArrayListWidget, just seems like a lot of work! And a lot of unkowns!

And now there is adding operators to procedurals... I don't see a way out of doing a custom graph editor. I have some thoughts on it, but I wanna be sure we exhausted native customization as much as we can.

JenusL commented 5 years ago

Do we really need to hack the RenderPass.spdl to add the port? Because there's no need for an ArrayListWidget as there can only be one target operator on options.operator and I would be fine without anything in the PPG. Every operator node has an input array of other operator nodes but there can only be one attached to the scene.

As for adding operators to procedurals. Couldn't we just add a material on a procedural. Yes this would mean that Materials would get the operator port as well. We could simply extend the workflow stated here: https://docs.arnoldrenderer.com/display/A5SItoAUG/Procedurals#Procedurals-Materials

Here's a way we could extend it: Scene_Material or Procedural_Material: Do nothing (as it is now) Any_Material: Shader and Operator exported. Any_Operator: Shader as is. Operator is exported. So if material name EndsWidth("_Operator"), only export the operator and keep the shaders intact.

Another way would be to add a "Pick Operator Material" parameter on the Arnold_Procedural PPG.

Just brainstorming here :)

caron commented 5 years ago

You're right we don't need to hack the RenderPass.spdl to add the ArrayListWidget, but I am not sure I like not having anything in the PPG signaling there is an operator attached. But since the user base is reduced heavily this might be an OK decision to make.

I am also not sure we need to add a naming convention for the material with an operator, we just check if anything is connected to that port and always export if something valid is connected. You could have the Scene_Material or Procedural_Material with an operator connection and you would not export a shader but do export an operator.

So if we add an operator port to all material and pass terminal nodes, we can use the type/family filtering to avoid connections we don't want. We need to test what happens to port data/connections when the plugin fails to load.


Now, regarding the other option of creating our own graph editor. We could serialize the graph data and store it on the ppg in a hidden parameter. Then add a button to launch the UI and load the data back. During export we parse that data.

The big issue is what method we use to create the UI. The obvious first choice is Qt, but that leads us to decisions about writing the UI in python or C++. If we used python we would need PyQtForSoftimage type features and need to have new requirements for using SItoA.

Then there is the licensing aspect of Qt we would need to double check on (since this code is still 'owned' by Autodesk). If we dynamically link Qt I think we can do pretty much whatever but if we want to minimize shipping Qt DLLs or requiring them during installation then we would statically link. The question I always had about statically linking was that SItoA is open-source but Softimage isn't. I would need to read the license closer and do some research and find out if releasing Softimage source code is a requirement if we statically link. Example situation...

https://opensource.stackexchange.com/questions/1479/can-plugins-for-closed-source-software-use-gpld-libraries

Sounds pretty icky right?

JenusL commented 5 years ago

You're right we don't need to hack the RenderPass.spdl to add the ArrayListWidget, but I am not sure I like not having anything in the PPG signaling there is an operator attached. But since the user base is reduced heavily this might be an OK decision to make.

Exactly my point.

Graph editor sounds like a fun project but I'm afraid it's way to much work. Not worth it for the small user base in my opinion.

Sounds pretty icky right?

Sounds really icky :)

caron commented 5 years ago

What is your schedule @JenusL ? I can work on this tomorrow my time (PST) for a few hours.

JenusL commented 5 years ago

I'm supervising on set for a TV show for the rest of the week so I have very limited time. Could probably answer questions though but will be pretty hard for me to write code. But here's my thoughts so far...

I don't know where the best place would be to add the new port type. Maybe in ShaderDer.cpp where the "closure" port is defined? What do you think?

caron commented 5 years ago

I am not sure we need an ArnoldOperatorsDef.py... we should be able to use the same auto registration and UI generation we already do but for operator node types. I will investigate this.

caron commented 5 years ago

I dug in a bit and have auto-generation of operator nodes working and wondered about if we wanted to create an arnold_operators.mtd or keep the metadata in arnold_shaders.mtd?

You can track progress here...

https://github.com/caron/sitoa/tree/feat/operators

JenusL commented 5 years ago

That sounds great! I haven't had the time to look at it yet but I will as soon as I have time. I would do a new metadata file for the operators so arnold_operators.mtd has my vote.

JenusL commented 5 years ago

I had a quick look now and most of it looks good. I added some comments to the commit. I also noticed code for the inputs on operators isn't done yet. It's a little bit tricky as it's an array of the new custom operator type. We might be able to use and extend the node array code I wrote for the toon shader to support operators. That would add this to the metadata:

[attr inputs]
soft.node_type STRING "operator array"
soft.inspectable BOOL false

I would love to write some of this myself but right now I'm pretty busy. Edit: I couldn't keep my hands away and I have something soon. Give me 24h and I'll PR it to you @caron.

caron commented 5 years ago

Ya, nothing is 'working' yet... I was going to go for getting one operator connected to the options.operator first. And I am researching how to handle inputs port which is a node array, using your work on the toon shader. I was also trying to decide if I could make operator type allow connections to/from reference type.

JenusL commented 5 years ago

node array is pretty easy now that I've done it several times. I'm not sure about the reference type. That will create a Pick reference for a Softimage object. Since I don't think it should be visible in PPG we don't really need the reference type, unless it helps a lot in the parsing of the tree.

set_parameters also has a string array type. I will add support for that as well tonight.

caron commented 5 years ago

is it easy? I mean does the code now exist so that only adjusting metadata will get the right behavior in the PPG? I can't seem to get the behavior you have in the toon shader with the lights parameter in our new operator nodes.

@sjannuz doing kick -info merge shows output: (null) why not have output equal something?

JenusL commented 5 years ago

@caron That's what I'm adding right now. What I did for the toon shader was a hack because that isn't an array in Arnold. I'm adding generic support for arrays now (only in shaderdef)

caron commented 5 years ago

I saw at your feat/shader_array_defs branch and wanted to mention I got array parameters working for auto ui gen. My changes are minimal and since I am not using a custom type for operators anymore and instead using siShaderDataTypeReference it works without any new custom metadata. Strings are looking like they work also and I don't believe I broke the toon shader OR the camera projection shader.

https://github.com/caron/sitoa/tree/feat/operators

JenusL commented 5 years ago

Yeah I saw that. I did a bigger change to make it even more flexible for the future. In my branch anything can be an array, including closures and what not. I also broke out the parameter_name to Label function.

JenusL commented 5 years ago

I also made great effort in keeping duplicate code to a minimum.

caron commented 5 years ago

Sorry @JenusL It wasn't obvious to me you were working on this. I didn't notice your comment edit from yesterday and you kept saying you were working on it and it went right passed me. I will let you finish and see if I can roll back my two commits so the merge of your feat/shader_array_defs should come in cleanly.

JenusL commented 5 years ago

No worries. It’s my fault that I’m always too quick to send my comments so that I have to edit it :)

caron commented 5 years ago

cool, I am working on adding new render options and starting to write the LoadPassOperator() function. I will stash it before merging your branch.

caron commented 5 years ago

@JenusL , why should we set 'inputs' to not inspectable? d9d14861b94bbcfed547208c2de2a773c252ca85

I think its nice to see the 'Add and Clear' buttons what do you think?

caron commented 5 years ago

@JenusL check out my feat/operators branch when you get a chance...

I am afraid we are hitting issues with using this undocumented feature that I outlined earlier. We don't seem to be getting good auto updates during IPR. We might be able to work around it but worst case we could still do the custom 'operator' node attached to the output shader stack.

I get this message all the time, but everything seems to work correctly...

[arnold] 00:00:00 420MB | [operators] the op defined in 'options.operator' is not an operator node

JenusL commented 5 years ago

@caron Yeah it doesn't matter about the inspectable. You can just right click and add/remove on the node as well. I mainly thought we should remove it when we had custom "operator" type where nothing new would pop up when you clicked add in the PPG. Would be nice to hide the Pick button tough.

As for the auto update in IPR.... I need more time to think about that and my time is very limited right now :( I haven't gotten that error message at all. Mind sharing a scene?

caron commented 5 years ago

@JenusL i only get the message with debug verbosity, again its working fine just seems like an odd message.

i understand your time is limited, i am going to bang away a bit more to try and get an update through the OnValueChanged event. if that doesn't work, i am going to try really quickly to get a custom operator node that plugs into the output shader stack and see if i can get it to work that way.

JenusL commented 5 years ago

So Arnold 5.3 is released with operator support on procedurals. From release notes...

  • Operator connection on procedurals: Operator graphs can now be connected to procedurals through the operator parameter. Only nodes in the procedural or nested procedurals are evaluated by the graph, where everything is performed relative to the procedural's name scope such as selection expressions. Furthermore, nodes created by the operators are put in the procedural's name scope. (#7747)

There is also some other Operator stuff that's new that should be added.

caron commented 5 years ago

There is also some other Operator stuff that's new that should be added.

Ya, I am hoping the auto ui gen will just catch the new 'include_graph' node.

JenusL commented 5 years ago

Just a FYI, Arnold 5.3 ships with it's first shader ever using an array parameter. So I will probably merge in my feat/shader_array_defs branch to my Arnold 5.3 branch to get that supported.

caron commented 5 years ago

so my feat/operators branch is kinda working...

so because of that I now need to test making a new operators node which will connect to output shader stack and see if changing parameters on the nodes works automatically.

JenusL commented 5 years ago

Ok cool. When do you think you have time to look at that? When I'm done with all the 5.3 stuff I could jump in on the operators as well. Would be nice to get everything out the door at the same time.

caron commented 5 years ago

I have been off and on it this week. I am on it now but I have no idea how long it will take.

caron commented 5 years ago

OK, as I expected and as @sjannuz probably already guessed... the custom 'operator' port on the material node doesn't get IPR updates. To test this I added a dummy operator node like the closure node and hook it up to the output shader stack and we get IPR updates as expected.

I still have an issue with a combination of disable and switch operator nodes. Whatever branch cooks first, I doesn't seem to undo that state when I change to a different branch. Might be an Arnold bug? I can try the new 5.3 core and see if it persists.

Lastly, this throws a stick in the spokes for handling operators on procedural nodes.

JenusL commented 5 years ago

Well we could just deprecate closure node and create a new node that have both a closure and operator port. The operator port would be ignored on anything but procedurals or pass output.

JenusL commented 5 years ago

@caron Maybe you could create a PR to my feat/a5.3.0 branch instead of to develop here. You can create it now so we can review and we'll merge it later. That way we could sync up easier before making a PR to this repository. We're working on a lot of the same parts of the code and that could lead to conflicts. Sounds good?

caron commented 5 years ago

i can do that, give me another hour

caron commented 5 years ago

just saw a commit on your feat/a5.3.0 branch about bumping the minimum version, i didn't know they had released a bug fix version. so i looked at the fixes and i saw this...

8215 Unable to enable nodes that were disabled in the first render pass

this looks like something i was running into and thinking it was me! checking that now and then doing a PR

caron commented 5 years ago

BTW, I did run the testsuite and it is 96% passing. The ones failing are related to texturing/projections and many are outright crashing and not failing the image diff.

JenusL commented 5 years ago

Ok thanks! I haven't gotten to the test suite yet with 5.3. Will fix that later. Did Arnold 5.3.0.1 fix the issue you had?

caron commented 5 years ago

Yes it did! :)

This is from the release notes...

8215 Unable to enable nodes that were disabled in the first render pass

I thought it was an IPR bug but it was just the disable node acting funny the first render, any future updates wouldn't allow it to be enabled again.

JenusL commented 5 years ago

Operators support is included in #68

JenusL commented 5 years ago

A very common thing to do with operators is to switch out shaders on nodes with a set_parameter node. This is currently very hard since the shader name in Arnold has some uniqe suffixes added. I'm working on a lookup function / translator that will make it possible to use the Softimage name.

JenusL commented 5 years ago

Done with the shader translation in https://github.com/Autodesk/sitoa/pull/68/commits/25a04f3774d5ce21eace1218a3714ae2c1633ed5. I also updated operators test.

JenusL commented 4 years ago

Closing this now. If we want to do Operators on procedurals sometime we can open a new ticket then.