Refactor with property sets, internalized shader params, define/include based modular shader, and a new UV distortion effect

[FishOfTheNorthStar]

This update has two main goals, one is to remove any responsibility for storing and maintaining core Terrain3D parameters off of the glsl side and into the material with c++. This allows more flexible presentation of the options to users, and a more solid and dependable system of serialization, in my experience. The other is to implement shader defines/includes. Also, I added a very basic inline help system and a UV distortion effect.

Towards the first goal, a couple dozen settings were made internal ("_" prefix) in the shader, and management of those settings was established in Terrain3DMaterial.cpp. Almost all code related to those settings is heavily templated using macros defined in __PROPSETS.h, which inline the same end-result functions from a much shorter set of inputs. Sets of properties are grouped by what they do, and each group has it's own set definition file. Typically, 100% of the code involved in those properties exists within that one file per group, so that should make it a lot easier to compartmentalize new code additions, fixes and changes. Also the main cpp file was just getting very long, especially with a bunch of new properties to track. Now it's much shorter. So creating a new group of properties in the Material resource is basically 'copy and paste the file, then in that file, search and replace a 'property group' id, and a 'property group readable label', and that's about it. Adding new property sets does require recompiling, naturally, but it does not involve adding any new code directly to terrain_3d_material.cpp

The other goal of this update is to implement a define/include system to replace the previous insert system. That is ready for initial testing and review, as a WIP.

I'm gradually migrating another, more heavily customized branch's code into this one, and all of that code is built around a system like the above, so if this looks good we can use it as a framework for the other things like strata gradients, elevation lines, auto-vegetation, shore/tree/snow lines, rain/puddles, and the feature-based shader. Each of those is a property set and accompanying glsl shader snippet, and if enabled, they get added in like modules dynamically based on the current defines.

There were no updates to the help docs in this yet, I wasn't sure if there's automated systems for some of that or how you want to handle it.

But speaking of help docs, this update does also implement a bit of an inline, read-only text field wrapped in a foldout system of help-per-property-set, so we can get some documentation delivered within Godot, until they get the new updates in place to allow dynamically injecting the help data from extensions into the main app's help data, which afaik is not technically doable yet, but they're working on it. So each property set definition file has a function that defines a help text for that property set, and it's pretty easy to change and adjust, for now. 100% of this less-than-optimal help system can be removed by removing the define "__TEMPORARY_INLINE_HELP" from terrain_3d_material.h, so the very moment we can do it correctly, we could shelf this temporary fix.

I was trying to keep any changes to how anything actually renders output very minimal. It should produce, pixel for pixel, the exact same output, none of that changed it just got moved around. I do eventually plan to do things that might change a bit of that though, so i'd like those changes to be trackable and easier to manage by being within portioned include files. I did want to show how this module system would work out though, so i took the most minimal one i have made here, UV distortion, and put that in to show how it can be disabled and removed entirely from the shader code, and how it integrates into the code as a whole. I have others and they do a lot more but they rely on other systems that i did not implement for this because it was already so massive.

Speed and framerate seem improved all around but I didn't do any extensive testing. I changed one part of how it was doing the weight blending calculation that gangs up 4 ops into one that I think helped a bit, and naturally the defines help by completely eliminating the code of disabled options, a bit more thoroughly than the insert system was able.

So have a look, if you want to dial it in or make some adjusts I'm happy to help. Apologies if i'm making a mess of the posting history, I'm very new to github and don't really know how all of this works very well.

[FishOfTheNorthStar]

btw the vertex-normals-by-distance is a great feature and I'll be setting up a propset for normals soon with it in it. It was such a new feature I didn't have a propset made for normals yet. For now to make that option appear, enable custom shader.

edit: done see 8093a74 - added a dropdown to toggle between always pixel, always vertex, or by-distance (the default), and a distance range slider, to a new propset labeled 'Mesh Normals' in the inspector and 'normals_' as a property prefix so it's external interface variables are 'normals_quality' (the enum of Pixel/Vertex/By_Distance) and 'normals_distance', the range.

[FishOfTheNorthStar]

These are what the macro generated propsets produce for Godot properties, pretty reasonable. All current Terrain3D shader properties are extended to GD script etc and manage their own updates and/or changing of defines.

Also it seems the 'inline-help' does actually show up in the actual help, but it gets a bit mangled. Still readable and informative though, and better than nothing. Perhaps if it was much shorter, this would be acceptable.

[TokisanGames]

Thanks for all of this work and putting this together. This is a large update so is going to take some time to digest and review. One thing that would help is to separate restructuring and new features into separate PRs. You mention the alternate normal calculation modes, maybe uv distortion and other things. If the new features could be moved into a second PR branching off of this one, it will slim this down a little. In fact, if there are multiple new features and ways it can be broken up into more than two, that would be even better, such as 1) property sets, 2) internalizing shader params, 3) new shader options or however it breaks down.
Making PRs as atomized as possible is standard practice in Godot and other repositories.

Don't worry about extra commits, those can be squashed later.

Also there were some regressions and class restructuring recently pushed that changed the material class, so this can be rebased to incorporate the changes. You can force push anything into this branch and it will update the PR.

[FishOfTheNorthStar]

I can't figure out how to get these merged. I'm very new to github/git. It's telling me to resolve conflicts with command line and text editor. Many of the things it's calling a conflict are intentionally just not there in my code because they're no longer needed, or moved into another file. Mine is so different. When I have more time I'll try again. If there's any suggestions or reading material you (or anyone else) would suggest on the subject , that'd be great, thanks.

[TokisanGames]

Indeed, this is a major challenge with large PRs. Now you know why standard practice is to keep them minimal and incremental.

We have other changes coming to both CPU and GPU side, so this needs to be maintained until it is merged. The slimmer it is, the easier that will be.

Git is a large beast. It gets very complicated and will take years to master. But it is a very powerful system. In the meantime, I can help. I've gotten pretty good at it and I can control the branch in this PR.

For now, familiarize yourself with this:
https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html

Also, don't touch git merge, or sync on github desktop which is the same thing.

I see you added a CPU noise for collision on the world background noise. Did you see my note about get_intersection? That's already taken care of.

Finally, you should make a backup of your branch. Abort any rebase, checkout your branch, make a new branch. Now you have a tag to the original so if you mess up the new one during rebasing you can start over with the backup. When merged you can delete the merged and backup branches.

[FishOfTheNorthStar]

OK it still says there's conflicts but as far as I can tell there actually isn't. show_checkered() changed names, and all the show_xxx functions were macro'd, renamed, and moved into their own file (in propsets/_DEBUG_VIEWS.h). This is as much as I can do about the merge conflict resolution process with what I know so far, if there's more I can do let me know.

[FishOfTheNorthStar]

I had not noticed you replied. Yes thanks, I'll try that. I went through and kind of ground it out with a lot of copying and pasting. As it stands it's up to date. This is as minimal as I could get it btw. Gave it a lot of consideration after reviewing my other branch that's way more modified, trying to come up with a plan to get some of that implemented here, everything in this PR is basically the base minimum to make that happen, as much as I can tell.

And yes I saw your note about get intersection that's very interesting stuff. I'm not sure if it'll work for what I'm trying to do, but it'll come in handy somewhere I'm sure. I mostly need a performant CPU side way to determine where the ground is outside of region maps, because my game won't be utilizing region maps the way I expect most people do. I don't expect to do much collision detection, I don't use godot physics for much and when I do it's easily replaced with other solutions if necessary.

What I would ultimately like to do is implement multiple noise suppliers, and my plan going into that is to make sure that all of them can meet interface standards of 'provide a height given a coordinate' (as we basically do now, through a UV), but also 'provide that exact same height from CPU when given a matching world coordinate (or uv)'. I figure if I find myself wanting this feature, than others do too (I'm sure someone out there wants to place trees around the edge of their background world), so this would facilitate that.

edit: get_intersection() will be my model to follow when it comes time to create viewports offscreen, render to them, and deliver the results to the extension. I forgot that was there and it's a great reference for how to instance a subviewport, draw to it, and get the results from gd extensions, which has kind of baffled me so far. Great! That was the missing link for me with baking normals and generating textures, thanks. But one thing at a time...

[FishOfTheNorthStar]

I had an idea for a feature some of this CPU ground solving might come in handy for, I was going to wait until we were further along but I think you'll like this. 1) A brush in the editor that fades in a baked version of the current world noise gradually, like the raise brush but with a stencil that basically is 'whatever the world noise is right there'. 2) an alternate operation mode for the auto-shader control map so it controls both auto shading and world-noise infill, so places inside region maps could also have world noise, selectively.

I picture these additions as kind of down the road. TBH I don't use the editor much, so it's a lower priority for me. I think people would like it though. And I probably would use the second one, that's basically how I handle roads, paths, and building foundations now (fade into a clamped height from whatever the procgen wanted to put there, over some distance)

[FishOfTheNorthStar]

rebased and squashed as much as I could get it to. still one junk commit log entry it won't let me remove but I reworded it so maybe it's ok.

[TokisanGames]

I mostly need a performant CPU side way to determine where the ground is outside of region maps,

get_intersection() is that, faster than the noise functions I'm sure, and far simpler.

What I would ultimately like to do is implement multiple noise suppliers

I want to move towards GPU based editing and noise generation on a compute shader. Having all of the noise generation on the CPU I think is unnecessary and slow. A compute shader will provide the exact same data to both the CPU and rendering GPU without duplicate generation code to maintain.

1. A brush in the editor that fades in a baked version of the current world noise gradually

Yes this would be great. Or maybe the above compute generation will already take care of that. Anyway we can discuss that in other tickets. Let's not get ahead of ourselves or distracted.

[TokisanGames]

OK it still says there's conflicts but as far as I can tell there actually isn't.

It is correct. It cannot be merged in at the moment because it isn't properly rebased.

This is usually the result of get merge which shouldn't have been used. Looking at the Files changed tab, I see there's code in the PR from my commits, which shouldn't be listed in your PR as changes. e.g. all of the changes to terrain_3d_storage., terrain_3d_texture_list. and more. It should only have your changes.

I can fix this PR, however let's try a different approach. Bear in mind, the goal isn't to get all of your changes into one PR. The goal is to make incremental steps in that direction. It might take 5-10 PRs to get there.

I hope you understand that I can't merge in thousands of changes that I don't yet understand (so can't support), haven't thoroughly tested, and then push that on to hundreds of users. We have to take it a step at a time so I can digest it, test it, and support it.

Here's a good example. Recently I discovered two bugs I had made months and weeks ago.

1. This commit had a bug (+232, -49 lines), here is the fix.
2. This commit had a bug (+153, -169 lines), here is the fix.

Take a look at both first commits. Imagine how long it would take to go through the hundreds of lines changed and find the bug in code you're unfamiliar with.

Now imagine you're in my shoes. We merge this PR in, and 1-6 months later discover this commit was the cause of a subtle intermittent bug, (e.g. under certain conditions black terrain, no error messages). The original author has moved on. Now you need to pour over 3400 changed lines of code that you didn't write to determine where the bug is. That would be a nightmare. a hundred lines or so is reasonable.

I want to incorporate your improvements, but as you said, one thing at a time. Let's take it incrementally.

For now, let's leave this PR as it is for reference. If I need to fix it later, I will. Looking at it though, I'm sure it could be broken down into 5+ separate PRs. For instance, we don't need UV distortion, online shader help, alternate normal calculations, grouping changes in the inspector, or a CPU version of the noise in this PR. Those should all be separate PRs.

What would be a good first PR is replacing the INSERT functionality with the shader defines, and that's it. No cosmetic changes, no new features, no renames except as needed, no WIP items, no property sets if possible. Just a simple PR that begins laying a foundation for the future.

Then the next PR lays the next foundational layer, and so on.

So, you have this PR and branch as a backup. Here's what I suggest. Start with the current main branch, then make a new branch. From this branch start copying over the code for only the INSERT removal and conversion to #defines at the most basic level, hopefully without the property sets system. Then make a new PR with just these minimal changes. It will probably be 100-200 lines instead of 3400. Then we can look at the next addition in a separate PR.

How does that sound?

[FishOfTheNorthStar]

Yes I can imagine the idea of all this dropped on your lap out of the blue (and the long term support commitment that could come with it) is face-meltingly intense at this volume.

I hope you'll try it out, I think you'd find it's much more cozy and intuitive, runs faster, is easier to customize, and you can actually access shader params from gdscript now without jumping through any weird hoops, and you can toggle stuff on and off.

I really don't know how to put the include/defines in there without the propsets. I tried to find a way, because I know the propsets part is particularly a big jump. But the way the defines work, either you always send all the uniforms, or you define them in and out as needed (and that absolutely does work better), but there's additional upkeep and management involved.

Take the 'Tinting' options for example. Now in this branch, you can toggle that on and off, and all traces of it are removed from the actual compiled shader code (but still displayed inside the shader text, as grayed out).

So lets say a user toggles that off, cause they want to see if turning it off helps their framerate (spoiler: now it does). So the define/include system says 'ok, no tinting, so no tinting uniforms, no tinting anything in there at all, no problem', and it yeets all that right out of the code. Now the user saves, or triggers reserialization somehow, and the shader-params-serialization system you're using currently looks at the uniforms and saves the ones that are there... except none of those are the Tinting options, because those are gone. Which is fine, because they're not being used... until the user turns Tinting back on, expecting their previous colors and options to still be retained. But they're gone, and need reset, because those options only existed within the shader. To me that is brittle, it's hard for me to feel confident about, as a developer. And to the users it must be frustrating too, trying to just toggle an option off and it completely resets.

For this and similar reasons, I think the include/define-system and managed-internalized-settings go hand in hand, they just have to.

And it's better that way, because the shader never really had any business having a UI anyways. Shader should stick to what it's good at, shading. However, the Resource class is made for managing reams of weird options, so it feels very backwards to me to basically knee cap the Resource class and put the shader-cart in front of the resource-horse. Shader should answer directly to the Material resource and there should be no need for anything to adjust or interact with the shader directly, externally. But as it stands, if I wanted to change that tint color from GDscript, it's going to get a little weird getting a reference to the currently loaded shader, finding the exact uniform name, and doing it the set_shader_param way. And there's really no need for it. Now there's just myTerrain.get_material().tinting_macro_variation1 = myColor, and it will update what needs updating behind the scenes, abstracting all the code-smell away from users that probably don't know or care what we specifically named that color uniform in the shader. They shouldn't be baking those kind of strings into their code anyways, that creates a nightmare scenario trying to ever change it in the future. The function based interface these propsets provide corrects that, users will never need to mess with those Terrain3D internal shader params directly, and we're free to rename (or even remove) those variables at any time without breaking anything for anyone.

So after reviewing these considerations, I decided that my top priority would have to be a way to internalize those shader params into the material class. Except now we get to a point you mentioned, supporting lots of weird new code. Without those propset macros, the internalized settings I added would have added hundreds, maybe thousands of lines of very-boring-code, just lots of the exact same function over and over. To me, that puts you farther back in the support game, long term. And the actual propset macro files are dead simple, and help a lot in the support area by making it easier to manage the code, and having very intuitive names that are grouped by function, and even the inline-help, as basic as it is, it legitimately is helpful. That means less support tickets.

Have a look, and definitely try it. Despite how much of it there appears to be, I'm sure you'd parse through the additions in no time. It's almost entirely the code you already had but organized more. The only place I'd say it gets a little weird is in __PROPSETS.h, it's like we boiled all the complexity out of the other stuff and compressed it all into that one fairly short file which you should never have to do much with at all. Once it's in place and functional (as it is here), you can basically forget about it. But yeah, you might need some strong coffee for __PROPSETS.h. It's short, but potently weird.