Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework Node UX #348

Merged
merged 273 commits into from
Sep 15, 2022
Merged

Rework Node UX #348

merged 273 commits into from
Sep 15, 2022

Conversation

pragma37
Copy link
Member

@pragma37 pragma37 commented Jul 8, 2022

This branch aims to improve the Nodes user experience with the help of @Kolupsy.

The goal is to make them work closer to the built-in Blender nodes and the MaltShadingEssentials plugin, while maintaining the convenience of declaring nodes from pure GLSL functions.

In the cases where auto-generation from GLSL is not enough, BlenderMalt specific nodes will be implemented.

Feedback is welcome.

Tasks:

  • Support META GLOBAL properties (Meta properties that apply to all the functions/structs of their file).
  • Support node categories instead of generating them from the GLSL path.
  • Support subcategories for generating nodes with a drop-down selector (like the Mix or the Math nodes in EEVEE).
  • Support label metaproperties (ie. Custom names for nodes and sockets in the UI).
  • Support enum properties.
  • Support min and max metaproperties.
  • Support Slider subtype.
  • Improve the design of the built-in nodes.
  • Allow access to internal nodes for advanced use cases.
  • Support shift+a search functionality for subcategory items.
  • Find a way to allow plugins and custom libraries for specific node tree to declare nodes in the same categories as the buil-in ones.
  • Update the generated nodes documentation to reflect the new metaproperties.
  • Use the default Blender socket colors where possible.

Known Issues:

  • Custom libraries for specific node trees are not yet supported. (Node Tree Local Libraries are deprecated)

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 8, 2022

Find a way to allow plugins and custom libraries for specific node tree to declare nodes in the same categories as the buil-in ones.

Here is my idea for that: either constantly unregistering/registering the categories or creating a global pool of all node items accumulated from Malt itself and plugins and replacing the draw function of the categories when that pool updates. This is how I add node items to blenders default node categories.

@pragma37
Copy link
Member Author

pragma37 commented Jul 8, 2022

When replacing the draw function you lose the search functionality, right?

Seems like the items parameter can be a callback that receives the current context, so that can be a good option.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 8, 2022

When replacing the draw function you lose the search functionality, right?

You can only search the node items of a node cateogory that returns True when polling. Specifically NodeCategory.items( ) (sorry items is what I meant before, but accidentally typed draw)
So replacing the items function to return a generator with your own node items makes those items searchable. Also NodeItems AND subclasses count towards this so creating NodeItem subclasses with icons for example is possible too

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 8, 2022

Support subcategories for generating nodes with a drop-down selector (like the Mix or the Math nodes in EEVEE).

This is checked but for me this has no effect. The hash functions for example are still all separate and have no dropdown menu.
Is this still in progress or was it supposed to work already?

image

@pragma37
Copy link
Member Author

pragma37 commented Jul 8, 2022

It works, check Node Utils > Float, for example.

Those multiple hashes is because this new UI is not tracking function overloads yet (the old did).

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 8, 2022

Ah yeah I just noticed that some actually do already have dropdowns.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 9, 2022

I found an issue with the subcategories. When the enum updates you need to recompile the graph for the change to take effect (an issue that pops up a lot in MaltSE). I cant commit the change though because I need to request access for GitKraken to push to the repo (request now pending).

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 9, 2022

Support shift+a search functionality for subcategory items.

Not sure if we can do this, as mentioned before, the search function lists all node items in categories that can be polled. So I think the only way to make subcategory items be their own search items you would need to create and display a 'Misc' node category that contains all the subcategory items as their own items.

@pragma37
Copy link
Member Author

I've added support for enum properties.
There's not any example in the branch, but this is how it can be used:

/*
    META
    @color: subtype=ENUM(Red,Green,Blue); default=1;
*/
vec3 test_enum(int color)
{
    switch (color)
    {
        case 0: return vec3(1,0,0);
        case 1: return vec3(0,1,0);
        case 2: return vec3(0,0,1);
    }
    return vec3(0);
}

There are probably some things left to fix and tweak, but I think all the major features needed for the new API are already there.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 18, 2022

for the min/max parameters. The 'bpy.types.UILayout.prop' function has a 'slider' keyword argument that we can use to display the parameters with both min and max as sliders. But I dont see any way to retrieve the malt subtype of a function input to reliably set that up. Where does the subtype go when its read by the parser? That part of malt is still pretty much a blackbox to me.

@pragma37
Copy link
Member Author

All meta properties are inside a "meta" dictionary in their function/parameter.

If you add

import pprint
pprint.pprint(functions)

to the preload_menus function in MaltNodeTree.py you can see the whole info.

This is how a single function looks like:

 'Node Utils - vec3 - vec3_mix_float': {'file': 'Node Utils/vec3.glsl',
                                        'meta': {'category': 'Math',
                                                 'label': 'Vec3 Mix Float',
                                                 'subcategory': 'Vector 3D'},
                                        'name': 'vec3_mix_float',
                                        'parameters': [{'io': 'in',
                                                        'meta': {'label': 'A',
                                                                 'subtype': 'Vector'},
                                                        'name': 'a',
                                                        'size': 0,
                                                        'type': 'vec3'},
                                                       {'io': 'in',
                                                        'meta': {'label': 'B',
                                                                 'subtype': 'Vector'},
                                                        'name': 'b',
                                                        'size': 0,
                                                        'type': 'vec3'},
                                                       {'io': 'in',
                                                        'meta': {'label': 'Factor'},
                                                        'name': 'factor',
                                                        'size': 0,
                                                        'type': 'float'}],
                                        'signature': 'vec3 vec3_mix_float(vec3 '
                                                     'a, vec3 b, float factor)',
                                        'type': 'vec3'},

@pragma37
Copy link
Member Author

pragma37 commented Jul 18, 2022

The GLSL parameters are converter to MaltParameters inside MaltNode.setup_sockets:

if isinstance(type, Parameter):

And then sent to the setup function of MaltPropertyGroup:

def setup(self, parameters, replace_parameters=True, reset_to_defaults=False, skip_private=True):

In the case of the subtype, it always gets stored as part of the MaltPropertyGroup rna dictionary:

rna[name]['malt_subtype'] = parameter.subtype

So the info was already there. It just had to be retrieved from the draw function:

slider = rna[key]['malt_subtype'] == 'Slider'

/*
    META
    @factor: subtype=Slider; default=0.5; min=0.0; max=1.0;
*/
vec3 test_slider(float factor, vec3 a, vec3 b)
{
    return mix(a,b, factor);
}

imagen

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 19, 2022

oooh. You know what? I always thought that these two methods of getting custom property meta data were the same:

image

But as you can see they are clearly not. I was always using the lower one in my tests. Maybe should have looked at the code instead of goin data mining in blender 😅

@pragma37
Copy link
Member Author

pragma37 commented Jul 19, 2022

I've added a new Input file in Node Utils that implements new nodes more in line with the Cycles/EEVEE/ShadingEssentials style.
What do you think?

@Kolupsy
Copy link
Collaborator

Kolupsy commented Jul 19, 2022

seems like we ended up working on the same thing now 😅
I was gonna wait for the poll to end but I guess we can estimate the result of it? What I made is basically the same, I do have more outputs on the geometry node though. Also putting those in a separate file is smart. I did not do that. I will push my version then we can compare...

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 8, 2022

Ah sorry I was going to rename it. It's a correction factor that brings the output range to 0-1. Before they were a bit lower. It's not that noticeable for the 4D noise but 2D and 1D didn't have very good value ranges so I added this. I know blender uses its own set of magic numbers to correct for this.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 8, 2022

maybe a name like 'range correction factor' would be better?

I don't think is worth adding a hardcoded node just for this.

Yeah I would like to avoid it as well if possible. Custom python nodes maybe belong exclusively into plugins to separate them a bit.

Another option would be to simply put regular and tiled options in the subcategory:

Thought about that too. Might be a good solution. Similar to this we could separate the node itself into a Tiled and Infinite version but this would add too many node category items again.

I think the voronoi node should have a randomness parameter. Do you think we should add one? I have not done that already because I know we are going to have to dance around the backwards compatibility issue since we dont want to keep a valid overload function that would work for old files

@pragma37
Copy link
Member Author

pragma37 commented Sep 8, 2022

I have refactored the noise functions into "c style templates", so we don't have so much duplicated code.
I've also made the 4d cell noise actual 4d and improved the code API a bit.

It's a correction factor that brings the output range to 0-1.

That's what the * 0.5 + 0.5 does, it remaps the dot product from -1...+1 into 0...1.
If you scale the result like that you are displacing the middle value and potentially setting the result outside the 0...1 range.
I'd rather add a contrast value to the user-facing node if you find it necessary.

I think the voronoi node should have a randomness parameter. Do you think we should add one?

Sure! As long as we make sure to keep a cell_noise and a cell_noise_ex function with the old signatures, there shouldn't be any compact issues.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 8, 2022

That's what the * 0.5 + 0.5 does, it remaps the dot product from -1...+1 into 0...1.

I am aware that this brings the value WITHIN the 0-1 range but it's actually only using a range that's more like 0.2-0.8. The magic multiplier was added to make sure that the entire 0-1 range is being used for the resulting values. Maybe there is a better way to improve this though. But I am not sure if you can do it analytically (instead of trying different values until you find one that works reasonably well which is what I tried)

@pragma37
Copy link
Member Author

pragma37 commented Sep 8, 2022

The important point is that if you want to remap the range you should use * magic_number + magic_number, if you do * magic_number + 0.5 the mean value is no longer 0.5 gray.

But I am not sure if you can do it analytically

The analytical way is to assume that the dot product can actually be 1 or -1 even if it's rare.
That's why IMO is better to leave it outside the core algorithm and add a node parameter (even if by default we add some extra contrast).

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 9, 2022

The important point is that if you want to remap the range you should use * magic_number + magic_number, if you do * magic_number + 0.5 the mean value is no longer 0.5 gray.

from my crude tests I was able to confirm that with the adjustment, the mean is still 0.5. The variance increases for sure but the mean is still 0.5.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 9, 2022

I have refactored the noise functions into "c style templates", so we don't have so much duplicated code.
I've also made the 4d cell noise actual 4d and improved the code API a bit.

looks interesting. I have never seen something like this for shader programming. Do you know how I can make VSCode not freak out? It is current not very happy with this arrangement 😅

I assume installing a VSCode extension should do the trick?
image

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 9, 2022

alright I looked up what the noise node in blender does and it has the same 'issue' that low values are almost never lower than 0.2. Since I never noticed that the support of the blender noise node is not actually 0-1 range but 0.2-0.8 instead I think it would not be too much of an issue to do the same for our noise. I think we could leave it how it is now (even though I am having trouble understanding this c-style layout now)

@pragma37
Copy link
Member Author

pragma37 commented Sep 9, 2022

Do you know how I can make VSCode not freak out?

I use the same settings that Malt sets when Setup VS Code is enabled.

This is what my .vscode/settings.json looks like:

{
    "files.associations": {
        "*.glsl": "cpp"
    },
    "C_Cpp.default.includePath": [
        "./Malt/Shaders",
        "./Malt/Pipelines/NPR_Pipeline/Shaders"
    ],
    "C_Cpp.default.forcedInclude": [
        "./Malt/Shaders/Intellisense/intellisense.glsl",
        "./Malt/Pipelines/NPR_Pipeline/Shaders/NPR_Intellisense.glsl"
    ],
    "C_Cpp.autoAddFileAssociations": true,
    "C_Cpp.default.cppStandard": "c++03",
    "C_Cpp.default.compilerPath": "",
    "C_Cpp.default.browse.limitSymbolsToIncludedHeaders": true,
    "C_Cpp.errorSquiggles": "Disabled",
    "python.analysis.extraPaths": [
        "./Malt/.Dependencies-310"
    ],
}

I am having trouble understanding this c-style layout now

It's essentially the same as a macro. Instead of doing something like this:

#define SUM(array, result)\
{\
    for(int i = 0; i < array.length(); i++)\
    {\
        result += array[i];\
    }\
}\

int a[10] = int[10](/** init **//);
int sum;
SUM(a, sum);

You put the macro content in a separate file:
(SUM.inl)

for(int i = 0; i < array.length(); i++)
{
    result += array[i];
}

and then:

int array[10] = int[10](/** init **//);
int result;
#include "SUM.inl"

The generated code is the same, but you get better compiler errors (with actual file and line number) and avoid the pain of adding the \ at the end of each line.

BTW, you were right that the correct way to remap the value is * remap_scale + 0.5 and not * remap_scale + remap_scale. 😅

@pragma37
Copy link
Member Author

I finally updated the shading nodes:
imagen

It's hard to settle on something when there's not a clearly defined "right" way to go, but I think these provide a good default.

With this done, I think the branch is ready to merge?

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 13, 2022

since I remember you wanting to add a smoothstep node, I went ahead and came up with a generalized easing function that behaves similarly to smoothstep but is parameterized. Its properties are that it works between the value range 0-1 where the derivatives at 0 and 1 are either 0.0 or 1.0. Also ease(0.5, x)=0.5 and and ease(x, 0.5)=x
I think this would be very useful to get more varied interpolations of values

edit: not sure if the thing about the derivatives is even true

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 13, 2022

throwing in smooth min and max for good measures. I think those two functions have been long overdue for implementation xD

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 13, 2022

It's hard to settle on something when there's not a clearly defined "right" way to go, but I think these provide a good default.

looks intuitive I think. Layering the shading models like that is a good idea.

With this done, I think the branch is ready to merge?

oops completely read past this. I have added some more changes so if you want to approve them then I think we could merge this into development.

@pragma37
Copy link
Member Author

oops completely read past this. I have added some more changes so if you want to approve them then I think we could merge this into development.

I feel about the easing functions kind of like I felt with blend functions.
We should add them, but I would prefer to take some time to do research and make sure we get the design right.

For example, I think it would be more useful if easing modes were part of the map range node, smooth min/max may be better if integrated directly into min/max nodes, all types should support them (not just floats), the Common/Math functions should probably be implemented as macros...

So I think it would be best if we leave them out for now, so we can merge this asap.

@Kolupsy
Copy link
Collaborator

Kolupsy commented Sep 13, 2022

all types should support them (not just floats), the Common/Math functions should probably be implemented as macros...

So smooth min and smooth max would be applied element-wise in the case of vector inputs?

I think it would be more useful if easing modes were part of the map range node

the reasoning behind this is that since easing functions require a fixed range of 0-1, this would make them more useful as part of the map range node?

So I think it would be best if we leave them out for now, so we can merge this asap

Sure, its just additional shader features after all and not critical to the node UX but maybe we should still look at them very soon.

@pragma37 pragma37 merged commit 4846720 into Development Sep 15, 2022
@pragma37
Copy link
Member Author

Merged! 🥳🥳🥳

So smooth min and smooth max would be applied element-wise in the case of vector inputs?

Yep, that's how the min/max functions work too.

the reasoning behind this is that since easing functions require a fixed range of 0-1, this would make them more useful as part of the map range node?

Yes, don't you think so?

@pragma37 pragma37 deleted the node-ux branch September 15, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants