MaltShadingEssentials icon indicating copy to clipboard operation
MaltShadingEssentials copied to clipboard

Node Tree types are not taken into account

Open pragma37 opened this issue 2 years ago • 6 comments

All the nodes are declared in the EssentialsNodeCategory, and this only polls whether the node tree is a MaltTree.

The nodes always can be added to any node tree, without taking into account the node tree type, the node tree language or even the active pipeline.

This means you can add shader nodes to python node trees (Render and RenderLayer). It also prevents Light shaders from compiling since it uses functions only available in Mesh and Screen shaders.

pragma37 avatar Jun 23 '22 18:06 pragma37

I have been putting this issue on my mental backlog because this would be part of a larger restructure that would make the plugin more robust and constistent. Similar issues are:

  • the plugin assumes the NPR Pipeline because it uses features that only work in that pipeline. Any other pipeline would likely break everything
  • I think nodes and node outputs should ideally be graph-specific meaning any node or socket should be marked as 'muted' or something in graphs that they dont work in (for example all the nodes in the Shader category dont make sense in a screen shader but some nodes do partially work in both mesh and screen shader but not all outputs ect)

I wanted to start tackling this issue once I figure out how to create wrapper nodes for custom pipeline nodes.

About light shaders: I have never used a light shader, which is why I have ignored this for now but I also have no idea how to prevent this compilation issue. I guess all functions in the shader libraries that only work in a mesh or screen shader need ifdef guards or something? Not sure what the best approach would be to fix this particular issue

Kolupsy avatar Jun 23 '22 20:06 Kolupsy

Admittedly, there's no way to tell what pipeline is currently active other than reading the pipeline path.

For disabling nodes from being added you can override the node poll() class method. But that would still show them in the add menu.

I think the easiest solution is to have a different NodeCategory for each graph type, and use the NodeCategory poll function to check if the graph type matches.

For the GLSL side, you can either use ifdefs (each shader type declares its own define: IS_MESH_SHADER, IS_SCREEN_SHADER, IS_LIGHT_SHADER) or put the functions in different directories based on their compatibility and load them accordingly inside register_graph_libraries().

pragma37 avatar Jun 24 '22 15:06 pragma37

I think the easiest solution is to have a different NodeCategory for each graph type, and use the NodeCategory poll function to check if the graph type matches.

this is what I would do too, yes.

For the GLSL side, you can either use ifdefs

But this still requires me to know which defines and functions work in which shader type? I think creating different directories for functions that are specific to a certain graph type seems like the most logical way On the same subject, is there a define that lets me check if the NPR Pipeline is active? ive seen NPR_FILERS_ACTIVE but I dont think this covers all cases iirc

Kolupsy avatar Jun 24 '22 15:06 Kolupsy

But this still requires me to know which defines and functions work in which shader type?

Sure, but there's not any other way around it.

It's not that hard, though. Mesh has access to the NPR Mesh, Shading, and Filters API. Screen has access to NPR Shading and Filters. And Light doesn't have access to any NPR Pipeline specific API.

Everything else is always available.

On the same subject, is there a define that lets me check if the NPR Pipeline is active?

Nope, but since you're already polling the pipeline in the PipelinePlugin, the Malt part of the plugin would never load otherwise.

pragma37 avatar Jun 24 '22 18:06 pragma37

Ok thanks a lot, that makes sense. I will try to come up with something for that. I just made sure all the ShaderFunctions files have their proper includes which should make it a bit easier to identify which functions are limited to certain graph types

Kolupsy avatar Jun 24 '22 19:06 Kolupsy

It turned out to be just very few functions so I just did the opposite of what I was going to do: 9c7713d91aea0e65566a91a1b7bac3cd4c4e08c7

This leaves the still remaining big chunk left that is creating sensible categories for the different graph types

Kolupsy avatar Jun 24 '22 20:06 Kolupsy