PR attempt # 3

[donn-xx]

Paddy,
I have now burned close to 5 hours in git and I am done in. After rebase went haywire and trying all kinds of ways to synch my 4.0 to yours, I deleted my fork, reforked and started over.

I hope this new PR is usable.
dbat/demos : I also made 2d demos of all the nodes I could.

dbat/nodes : Added nodes, as well as updated UVTiler, UVRotate and the Compare (new name Color_Compare because there was a Compare node already).

dbat/nodes/Filters : Not sure about "Filters" name for things like blur. Let me know.

dbat/nodes/Shapes : Find two of the shape nodes you already made with an added feather option. Would that be useful to extend to all the shapes?

dbat/nodes/WorldNormal : A cool shader I learned from Arnklit (Kasper).

dbat/nodes/library : The LizardShaderLibrary for your consideration.

Look for README.txt files in the folders where I have extra info.

HTH
/d

[paddy-exe]

Here's some feedback about the Nodes you integrated so far. Btw great work!

LoD Blur

This node should be called "LoD Blur" (Level of Detail)
Two inputs should be renamed to "UV" and "Blur Amount". The Output should be named "Output" as well.

Color Compare

This node should be probably named "Color Mask" (without the underscore - just as the one above and all the others)
The input "Fuzz" also renamed to "Blend Amount". I changed my mind about the color inputs and the output. The top input should be called "Input", the second one "Mask Input" and the Output port to "Output". I think this would make it clearer what the node does. Let me know your opinion on this.

Connector

Great node idea! The inputs and outputs should be the actual core port type names for clarity though. So sampler2D instead of "S".

Mask Blend

Here, the first input should be "Mask Input", the second "Blend Amount" and the third "Blend Fade". The output port to "Output".

Restore NormalMap Z

"UV in" to "UV" and you can leave the "Sampler" in the node title as well as the underscores.

UVRotate

Perfectly done

Shapes

The titles should be more descriptive like "Smooth Circle" and "Smooth Box". Also the "Feather" port to "Smoothness" or "Blend Amount".

TextureStack

"UV in" to "UV". Not so sure if we even need the Output names... or maybe just the first letters with "Out" after that like e.g. "A Out". also, for the functions in the node script: please use more descriptive names for the functions:

UVTiler

Should be names to "UVBrickTiler" instead. Also not sure but can you change x and y for the Tiling to be the opposite (so x actually changes the tiling in the horizontal direction and also for vertical and y? Then you can remove the "(Down, Across)". "rotation" should be captial "R".

General feedback

• I don't think that the Library makes much sense. The called functions are being optimized by Godot so there is no need for that. Also the description and issue methods aren't that useful in comparison to the standard description method.
• The examples should be using only the library icon and the Godot icon and no other if possible.
• The examples should be inside the examples scene. For the 3D one I might have to think of a good way to showcase them but that will have to wait for now

[donn-xx]

I agree with all your points. Will get on it.

PS - As to the titles - I have tried putting a space in, like "Two Words" and that causes a gdscript error. So I will just remove the underscores.

[donn-xx]

Version A seems better because it's smaller (I'd prefer a Material Maker style connector, but we can't :( ) but version B is as you suggested, which I think looks clunky. What do you think?

I will also move Scalar and Int down to be with the Vector types in a more natural grouping. Sort of -- I did this:

[paddy-exe]

I agree with all your points. Will get on it.

PS - As to the titles - I have tried putting a space in, like "Two Words" and that causes a gdscript error. So I will just remove the underscores.

In that case just remove them, exactly.

[paddy-exe]

Version A seems better because it's smaller (I'd prefer a Material Maker style connector, but we can't :( ) but version B is as you suggested, which I think looks clunky. What do you think?

I will also move Scalar and Int down to be with the Vector types in a more natural grouping.

you are right, the left one looks better. You should also change the Int to "Integer" then

[donn-xx]

you are right, the left one looks better. You should also change the Int to "Integer" then
Sorted.

[paddy-exe]

Btw for the future, it is probably easier to create more smaller Pull Requets than one HUGE one. But that is up to you in the end. It just makes it smaller to debug a small addition in my humble opinion

[donn-xx]

Btw for the future, it is probably easier to create more smaller Pull Requets than one HUGE one. But that is up to you in the end. It just makes it smaller to debug a small addition in my humble opinion

It's a function of the time I have to work. With the power outs we have I put much more effort into just getting stuff done. It's hard to explain what it's like having your day chopped into 3 or 4 hour stretches of time to code. I'll try to keep each PR smaller in future.

[paddy-exe]

Ah yeah sorry about that. I mean if this type of workflow suits you better, that totally also works for me. No worries there then. Just ignore my last remark then :)

[donn-xx]

Shapes

The titles should be more descriptive like "Smooth Circle" and "Smooth Box". Also the "Feather" port to "Smoothness" or "Blend Amount".

My question was rather should we alter your current shapes to include the "Smoothness" option rather than have another crowd of similar named shapes?

UVTiler

Should be names to "UVBrickTiler" instead. Also not sure but can you change x and y for the Tiling to be the opposite (so x actually changes the tiling in the horizontal direction and also for vertical and y? Then you can remove the "(Down, Across)". "rotation" should be captial "R".

I wish I could change the x and y ! I tried! It's beyond my mathematical chops to understand what it's actually doing. If you could take a look at this one, I'd appreciate it. The other mystery is when doing the random rotation while the brick-shift effect is happening causes some strangeness.

General feedback

* I don't think that the Library makes much sense. The called functions are being optimized by Godot so there is no need for that. Also the description and issue methods aren't that useful in comparison to the standard description method.

1. Do you mean the library name makes no sense or the entire library as a concept?
2. What do you mean by the called function are optimized? Are you talking about gdscript funcs or shader funcs?
3. I don't mind removing the issues from the description - my thinking was to inform the users of the nodes about potential issues - perhaps they might even be able to help. Otherwise the issues sit in a comment or on a website and no one ever sees them.

* The examples should be using only the library icon and the Godot icon and no other if possible.

Ah, okay - can you approve at least two others like a rock and moss texture*? We will also need their normal, occlusion, metal, yadda-yadda maps. There are nodes that can't be demoed without them.

• The ones I used were from Kasper's github, but he has no specific licence so we should find replacements.

* The examples should be inside the examples scene. For the 3D one I might have to think of a good way to showcase them but that will have to wait for now

Forgive me if I delay on that for a while. I have to be out of my house by the end of this month and then I am house hunting from some rented thing. I will be MIA for a while. I won't start mucking about in your example scene at this point.

Glad you like the new nodes so far! It's been fun.
Best,
/d

[paddy-exe]

My question was rather should we alter your current shapes to include the "Smoothness" option rather than have another crowd of similar named shapes?

You can replace the existing ones with your Box and Circle Shape version. No need for two versions.

I wish I could change the x and y ! I tried! It's beyond my mathematical chops to understand what it's actually doing. If you could take a look at this one, I'd appreciate it. The other mystery is when doing the random rotation while the brick-shift effect is happening causes some strangeness.

I will have a look at it.

1. Do you mean the library name makes no sense or the entire library as a concept?
2. What do you mean by the called function are optimized? Are you talking about gdscript funcs or shader funcs?
3. I don't mind removing the issues from the description - my thinking was to inform the users of the nodes about potential issues - perhaps they might even be able to help. Otherwise the issues sit in a comment or on a website and no one ever sees them.

1. The library as a concept. The shader functions are being optimized to be only once inside the shader code (even if they share the same function among different Visual shader nodes.
2. I'd appreciate it if you remove the issues first. If we can't solve them reliably I would mark them Experimental or not release them at all until they are stable.

Ah, okay - can you approve at least two others like a rock and moss texture*? We will also need their normal, occlusion, metal, yadda-yadda maps. There are nodes that can't be demoed without them.

Where did you get these textures? I think one of them is good with all different maps but why two of them?

• The ones I used were from Kasper's github, but he has no specific licence so we should find replacements.
  Agreed. I suppose we could even create some basic ones or take some from the MaterialMaker website depending on the license on the material.

Forgive me if I delay on that for a while. I have to be out of my house by the end of this month and then I am house hunting from some rented thing. I will be MIA for a while. I won't start mucking about in your example scene at this point.

Absolutely no problem. Take your time :D I don't intend to release the 4.0 version until I had time to implement the nodes for Sky and Water Shaders anyway.

[donn-xx]

You can replace the existing ones with your Box and Circle Shape version. No need for two versions.

Roger. I will try to add Smoothness to the other shapes over time.

I wish I could change the x and y !
I will have a look at it.

Tah.

1. The library as a concept. The shader functions are being optimized to be only once inside the shader code (even if they share the same function among different Visual shader nodes.

I don't mean to be difficult, but I don't quite follow your reasoning. Here's my last (I promise) attempt to defend the library concept:

1. It allows us, the programmers, to :
   a. reuse shader functions without having to copy and paste strings all over the place.
   b. if there's a bug in one of the shader functions (the const vars in the library) then we can fix it in one place.
   c. The library forms a really nice reference for us (and anyone) to see all kinds of functions in one file.
   d. It can offer common methods for use in the various node classes.
2. I don't see why this concept has anything to do with the final shader code optimizing etc. It's quite apart and a is a gdscript thing. The way it's implemented means there's only one copy of all the strings in memory.
3. That said, I do see that the library will be loaded into memory and will take some space at final runtime - I think... I am not quite sure how Godot handles that - does it run through the shader nodes and recreate the shader code every time you load a project, or is that code cached and only re-generated if you open the actual visual shader editor?

Does that make sense to you?

If you still thumb it down, can I implement a small version of it for those functions that are strictly used more than once? For example:

1. WorldNormalMapMixer.gd, MaskBlend.gd and NormalMapZ.gd all reuse the same functions.
2. UVRotate.gd and UVTiler.gd both share vec2_rotate().
3. Then there are the ubiquitous random functions that many nodes will need to call upon?

2. I'd appreciate it if you remove the issues first. If we can't solve them reliably I would mark them Experimental or not release them at all until they are stable.

I will remove the issues and such and return a simple string from _get_description() with only a description.

[re textures] I think one of them is good with all different maps but why two of them?

If you look at the world normal demo you will see it's mixing two sets of textures to get the effect. Their normals are mixed and so on. There will be further nodes that make use of ORM and other PBR techniques that will all require the full complement of maps. It's just worth finding a nice demo set that the godot logo can't cover.

I suppose we could even create some basic ones or take some from the MaterialMaker website depending on the license on the material.

For sure!

Absolutely no problem. Take your time :D I don't intend to release the 4.0 version until I had time to implement the nodes for Sky and Water Shaders anyway.

Cool. I am playing with ideas too and I'll try to keep my repo in a stable state.

Good luck with your exams.

[paddy-exe]

I don't mean to be difficult, but I don't quite follow your reasoning. Here's my last (I promise) attempt to defend the library concept:

1. It allows us, the programmers, to :
   a. reuse shader functions without having to copy and paste strings all over the place.
   b. if there's a bug in one of the shader functions (the const vars in the library) then we can fix it in one place.
   c. The library forms a really nice reference for us (and anyone) to see all kinds of functions in one file.
   d. It can offer common methods for use in the various node classes.
2. I don't see why this concept has anything to do with the final shader code optimizing etc. It's quite apart and a is a gdscript thing. The way it's implemented means there's only one copy of all the strings in memory.
3. That said, I do see that the library will be loaded into memory and will take some space at final runtime - I think... I am not quite sure how Godot handles that - does it run through the shader nodes and recreate the shader code every time you load a project, or is that code cached and only re-generated if you open the actual visual shader editor?

Does that make sense to you?

If you still thumb it down, can I implement a small version of it for those functions that are strictly used more than once? For example:

1. WorldNormalMapMixer.gd, MaskBlend.gd and NormalMapZ.gd all reuse the same functions.
2. UVRotate.gd and UVTiler.gd both share vec2_rotate().
3. Then there are the ubiquitous random functions that many nodes will need to call upon?

It seems like we were both wrong:

Here, I used both of the UV nodes you added that both use the uv_rotate function. There is a name conflict between both of them ( that is why there is only the basic texture shown -> visual bug). I had a look at the docs for the _get_global_code method and there is a warning about this as well:

But my reasoning is still valid that the library doesn't make sense then. I guess there is no optimization possible for Visual Shaders after all... Could probably create a proposal for that though.

If you look at the world normal demo you will see it's mixing two sets of textures to get the effect. Their normals are mixed and so on. There will be further nodes that make use of ORM and other PBR techniques that will all require the full complement of maps. It's just worth finding a nice demo set that the godot logo can't cover.

Yeah alright. This makes sense. If you get the permission from Arnklit or find another suitable alternative, we can do it like this.

Cool. I am playing with ideas too and I'll try to keep my repo in a stable state.

No worries :D looking forward to all the cool nodes you have planned as well!

Good luck with your exams.

Thank you! :D

[donn-xx]

Paddy, I was being super dense about the library thing. I see what you mean now. It has piqued my interest to see if I can't solve the problem, so ... rabbit hole time! :) We'll see.

[paddy-exe]

Same node, different insides. I renamed the old one, in your "Usability" dir, with underscores; I also added DEPRECATED to the class_name.

Since the 4.0 version isn't released yet I will just remove the node from the 4.0 branch and you can rebase your changes on top of mine.

[donn-xx]

The same goes for the UVTiler.gd node.

Would it not be easier for us to leave nodes in their own directory structures? The category already sorts them visually for the end user. This way we won't be stepping on toes and having to try remember what nodes are old and so on.

For example, we could have:
VisualShaderExtras/
../demos
..../resources
../nodes
..../dbat (bunch of .gd files)
..../paddy (bunch of .gd files)
..../foo (bunch of .gd files)

[donn-xx]

Paddy,
I had a bit of time and some electricity today. I have moved things around a little - in my fork, I removed my nodes from your main structure and put the all in dbat/nodes. I think this will assist in merging in future.

I managed to make a connector that is only in<-->out which is cool. I call it the One to Many. The other connector is now Many to Many. There is a demo.

I might not get more time in the month or two ahead to do any work I am packing my old house up and preparing to move 1000km north. It's a crazy time. If you need stuff done, it's probably better to clone my fork and manually pick and choose what you want from my donnv2 branch (the latest).

Best, until the chaos subsides.
/d

[paddy-exe]

The same goes for the UVTiler.gd node.

Ah, good to know.

Would it not be easier for us to leave nodes in their own directory structures? The category already sorts them visually for the end user. This way we won't be stepping on toes and having to try remember what nodes are old and so on.

For example, we could have:

VisualShaderExtras/

../demos

..../resources

../nodes

..../dbat (bunch of .gd files)

..../paddy (bunch of .gd files)

..../foo (bunch of .gd files)

The folder for demos is a valid point for when more scenes like 3D demo scene(s) will be added but I am 100% against fractioning the nodes inside contributors folders. It makes more sense to have the same structure for the nodes like they are structured in their respective category to have easier maintainability. This is not something I will ever change.

I hope you have a good move. About merging your PR though: You will still need to adapt your changes to the current nodes.

[donn-xx]

No worries re directory structure.

About merging your PR though: You will still need to adapt your changes to the current nodes.
I'm not sure I understand what you mean. Can you translate into dumb? :P I only speak dumb.

BTW - Here's what I learned about the repeating function names:

1. If custom shader node W returns a global func f() then all W's can do so, Godot makes sure there's only one func f() in the final string.
2. If W and custom node P use func f() then there's a name clash. Godot does not do a final sweep on the global output string.

Therefore it is necessary for all nodes which define global functions to ensure those functions are namespaced so they do not clash. The ShaderLib does this namespacing:

func _get_global_func_names()->Array:
	return ["compare"] # <-- return name of func to include
	
func _get_global_code(mode):
	return ShaderLib.prep_global_code(self) # <-- prepare the output code with the right names

func _get_code(input_vars, output_vars, mode, type):
	var code = "...stuff..."
	return ShaderLib.rename_functions(self, code) # <-- swap in the right names when calling funcs.

I could find no way to access the final string before it gets to the output node, so there seems no way to just remove all duplicates.

I thought I'd go through this specifically to see what you say about it. If we dropped the shaderlib, we'd still need some mechanism to ensure unique function names across different custom shader nodes.

How go your exams?
/d

[donn-xx]

I made a small change to the connector nodes. I had a brainwave and simplified them. I also renamed them to Route and RouteMany (using Blender's term for it). I have pushed to donnv2 but that is all. Out of time again!
/d

[paddy-exe]

I could find no way to access the final string before it gets to the output node, so there seems no way to just remove all duplicates.

Yeah I suppose it isn't possible after all. I would be in favor of dropping this then. I could imagine having a custom Material or Shader class that automatically deletes the duplicated functions but that would be over the top complicated.
How about renaming the repeated functions to $NODE_NAME$_$FUNCTION_NAME$ then?

[paddy-exe]

@donn-xx I updated the godot project to Beta 16 with the breaking changes in #13. You can rebase your changes on top of these then

[donn-xx]

Well done on passing you exams, saw it on Twitter. I am knee-deep in boxes and then I am moving house. Will try get back into this project soon as I can, but I can't say when I'll have a desk and Internet and all that. See you on the other side.

[paddy-exe]

Well done on passing you exams, saw it on Twitter. I am knee-deep in boxes and then I am moving house. Will try get back into this project soon as I can, but I can't say when I'll have a desk and Internet and all that. See you on the other side.

Thanks :D and yeah no worries!

[paddy-exe]

Bump :) just wanted to ask how it's going with this PR and if you are ready to pick things up again.

[donn-xx]

Hi Sir Paddy, I am finally in a flat and have got my pc setup. I will get back into it soon. I see you released a version as well as a new project for the code-only side of shaders. You're a busy mensch. I'll try catch up to where we left off, to remind myself what was what. I have a great forgettery! /d

…

On Mon, 20 Mar 2023 at 01:09, Patrick (FlameLizard) < ***@***.***> wrote: Bump :) — Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC7YGTHQPPL476NOWLXYITW46GY3ANCNFSM6AAAAAATQB7L74> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[paddy-exe]

Hi Sir Paddy, I am finally in a flat and have got my pc setup. I will get
back into it soon. I see you released a version as well as a new project
for the code-only side of shaders. You're a busy mensch.

I'll try catch up to where we left off, to remind myself what was what. I
have a great forgettery!

Sounds good👍🏻

[donn-xx]

Guten Tag Paddy,
I started refreshing my old brain this morning. I also read these PR messages again. You made one post that I don't understand:

Yeah I suppose it isn't possible after all. I would be in favor of dropping this then.
Dropping what?

I could imagine having a custom Material or Shader class that automatically deletes the duplicated functions but that would be over the top complicated. How about renaming the repeated functions to $NODE_NAME$_$FUNCTION_NAME$ then?
I am doing $NODE_NAME_$CATEGORY_$FUNCNAME at the moment just to make sure there's no overlap. I'll experiment with removing the category and see what happens.

What's your overall opinion about the code I pushed and the ShaderLib? Will you adopt the approach or is it a problem?

I had started converting a hex-tiling (non repeatable) shader before I moved, I will try to get back into that and get it into a node.

Do you think there is any overlap with this visual shader project and your new shaderFunctions project? If the node functions could somehow be changed into .gdshaderinc files, then we could share that one collection of files between both projects? Maybe a common folder under Addons?

I would have to experiment with #include, because in the last version of Godot I used there was no way to output a "#include" string from the _get_global_code() function. It just refused for some reason. Perhaps some kind of Unicode issue? I shall have another go.

Auf Wiedersehen,
/d

[paddy-exe]

Hey @donn-xx, I will have to reevaluate the nodes you pushed and test them out. I would also prefer if you could re-create the PR since the whole workflow of PRs is not to discuss integration but integration and merging the PR itself. Since this PRs has evolved very chaotically you are best to close it and open a new one (also a new one for one node at a time preferably).
There is an overlap of functionality between the two repositories but I don't currently plan to put the Function-Library as a dependancy here as it will only needlessly complicate the development.
I am also not a fan anymore of the ShaderLibrary here since this would also complicate the whole ordeal of nodes. This comes at a price of having duplicate functions that would have to be renamed according to their specific nodes so theres no namespace collision but I am happy to have that for the sake of simplicity.
Also, visual shaders are not made for optimisation but rather for artists who don't want to work with code.

[donn-xx]

Paddy,
Very well. I will re-work all my nodes thus; and remove the library.

I must say that I am finding the git workflow to be obscure. I'll just do my best in my fork for now and then ask you to clone/pull and review that. Perhaps you can then PR or merge from your clone, or give me specific instructions closer to the time.

If you need to discuss non-PR things then use my email: donn.ingle@gmail.com.

I'll close this PR now.