Add Signals and Improve processing

This draft PR branches off of #8. Only look at the last commits starting with f7965b087f3e59c77a573911662f59262f7d1708.

Currently implemented:

Sending signals on lod change, and unload
Removes disable_processing function call
Provides a demo of using signals to disable objects in game

It works well in my game, allowing me to entirely remove ProximityManager and ProximityClient classes I was using to hand out physics ticks, and disable objects (state_machines and AnimationPlayer) by using signals generated from MTLOD. Even on objects that do not have LODs, I just specified the node I placed the gdns script on as LOD0 and the parent script was able to connect and receive the signals just fine.

lod_changed signals are sent at all specified lod distances, even if the lods don't exist. So an object with only LOD0 and distances of LOD1:25, LOD2:50, LOD3:75, HIDE:100 will receive lod changes for 1, 2, 3, 4 at those distances. 4 means hidden, and they can do whatever they want at the other lods, such as disable animations and processing while the object is still visible.

As for the other lod types, they either blend and don't have hard distance levels, or if they do, the user can simply connect to the existing Spatial::visibility_changed signal.

A separate freed signal is sent immediately before the object is marked with queue_free(). The demo uses both signals.

I'm open to comments on this section.

Note current binaries are old; from #8

Todo:

Rework enabled and registered
Reduce processing during enter_tree and exit_tree and other optimizations
Add binaries
Update documentation

Question:

Why is ImmediateGeometry specified in the documentation and LODPlugin.gd? Nothing in the code uses IG's properties or functions. lod.gns can be attached to anything downstream from Spatial. It only needs visibility, 3D translation, and Node properties.

Makes sense to me.

Where are you seeing ImmediateGeometry in the docs? I see it in LODPlugin.gd but not in the main readme.

I think this might be because this is applied to the parent node of the car example, and I wanted to use something that inherits from VisualInstance (which you cannot directly create in a scene) that did not have a mesh and, without usage from code, was basically "blank" in the editor.

I would ideally use Spatial, but it does not have the AABB needed for screen percentage LOD.

New in this update (starting with 2b82ea4):

Enabled now works. There are some disabled objects in the demo. I also simplified registration and the number of variables a bit. LODManager is already set up and accessible to the object when it is going through ready. Objects can updated their AABB and multipliers and don't need dirty bits.
I replaced the split multi-inheritance model with a Component design pattern instead. The LODComponent is derived and templated to each LOD type. The shared variables are in the component, and more importantly it has pointers to the object and LODManager, so it is a bridge between the two. LODManager has arrays of the components, so now it doesn't need to worry about LOD object types, casting to a neutered Node*, or calling functions through the clunky gdscript call("function") interface.
I was trying to figure out how GIProbeLOD works, but it wasn't doing anything. It has user parameters: use_screen_percentage, which wasn't being used at all. Same w/ MMILOD and LightLOD. And the hide_distance user value was overwritten. I fixed it so these three types respect these settings.
When screen percentage is on, I think the screen size numbers are too small. GIProbe hide_distance calculates out to hiding at 357 units away, when it's a tiny square on my screen. They basically don't do anything until the objects are super tiny. In the demo, they were all invisible due to the 256 far camera clip, long before any of the three had any effect. I've extended the far camera clip, and now they all work w/o screen_percentage. Do you want to increase the defaults in the code, or just overwrite them for the demo?
tan_theta only changes when fov is changed, so now it's calculated only on set_fov.
fov & tan_theta don't need to be stored or calculated on every object, so they are stored in LODManager.

General notes:

I discovered Godot is quite limited with it's availability of AABB information. :/ I don't see ImmediateGeometry in the docs; only in the demo.
The signals are working quite well. I've been using them from the beginning without issue.
What do you think about renaming LOD to MeshLOD to match the format of the others?
After I worked on this update, I split up the changes into multiple commits so it's easier to see what the changes are. Some changes for a task may have been missed and pushed in subsequent commits.
With the current main tree, prior to this PR, when I debug the engine and quit a scene, I get this access violation upon closing. You can see in the call stack it is called from the LODManager destructor, which is empty. Do you know what might cause this? Even if I run LodManager.stop_loop() before closing, we get the same thing?

A few more change I want to do are:

remove_object: Currently when changing scenes and potentially thousands of objects are exiting the tree, it searches through the arrays for every object. Potentially iterating over every array, every index, thousands of times. I'd rather just unregister it, but leave it in the array, then schedule a garbage collection to flush the arrays once.
As for add_object, I'm thinking about pre counting the number of objects in _init, having lod_manager reserve the memory for the arrays. As it is, it pushes back each time, potentially rewriting the array thousands of times. I need to think about adding an object if it has been unregistered in enter_tree. I don't think it needs to be removed and readded to get it processing again. I think it just needs a tweak in lod_function
Also I need to test it more thoroughly with multiple scenes and many objects in my game.

Enabled now works. There are some disabled objects in the demo. I also simplified registration and the number of variables a bit. LODManager is already set up and accessible to the object when it is going through ready. Objects can updated their AABB and multipliers and don't need dirty bits.

At first glance, looks like it makes sense. I'm sure you'll get thorough testing on this end.

I replaced the split multi-inheritance model with a Component design pattern instead. The LODComponent is derived and templated to each LOD type. The shared variables are in the component, and more importantly it has pointers to the object and LODManager, so it is a bridge between the two. LODManager has arrays of the components, so now it doesn't need to worry about LOD object types, casting to a neutered Node*, or calling functions through the clunky gdscript call("function") interface.

Ah thanks! I tried to do this during my refactor but couldn't get Godot to play nice and settled for the minor amount of reuse I managed to get working.

I was trying to figure out how GIProbeLOD works, but it wasn't doing anything. It has user parameters: use_screen_percentage, which wasn't being used at all. Same w/ MMILOD and LightLOD. And the hide_distance user value was overwritten. I fixed it so these three types respect these settings.

It seems to be working for me in the original demo. Screen percentage takes precedence over the other variables, so it makes sense that hide_distance would be overwritten due to a hide range ratio. This might be a usability/description problem as well as defaults that are too small. To be honest I'm not sure what exactly you've changed since it looks about the same to me, including the behaviour, but if this version is easier to understand then 👍

These two in general are sort of "WIP-ish":

GIProbe is very reliant to engine source code changes to make much sense (so it's kind of niche, especially considering default GIProbe performance.... though it can be improved a lot through some relatively easy source code changes, but that's a whole different topic...)

MultiMeshInstance works decently, but I was hoping to get it reduce the count based on distance as well. I don't think I'll get around to it, particularly because the only way I could think of to do it at least somewhat elegantly would be if this is added, but I'm not sure if I'll bother to update it. Although there is still an issue that someone else could tackle to add such support. Might be a Godot 4 thing (if this is updated for it). Otherwise you could still use it for groups of objects that are close together.

When screen percentage is on, I think the screen size numbers are too small. GIProbe hide_distance calculates out to hiding at 357 units away, when it's a tiny square on my screen. They basically don't do anything until the objects are super tiny. In the demo, they were all invisible due to the 256 far camera clip, long before any of the three had any effect. I've extended the far camera clip, and now they all work w/o screen_percentage. Do you want to increase the defaults in the code, or just overwrite them for the demo?

It hides properly at 80 units for me in the demo, but those are indeed overwritten values. Sure, we can increase the defaults. I would increase to somewhere in the middle though since GIProbe transitions can be very jarring if they are too close.

tan_theta only changes when fov is changed, so now it's calculated only on set_fov.

👍

fov & tan_theta don't need to be stored or calculated on every object, so they are stored in LODManager.

👍

I discovered Godot is quite limited with it's availability of AABB information. :/ I don't see ImmediateGeometry in the docs; only in the demo.

Now I'm starting to remember... I believe I used ImmediateGeometry because you can effectively leave it "empty" and use it as a Node that has an AABB. When you have multiple objects in it, it also includes all of them in the AABB calculation. It's essentially an AABB wrapper for all children nodes, whatever they may be. This makes it useful as a parent for various LOD objects, which are switched based on that AABB.

In this case, multiple MeshInstances would be included in its AABB, so you would have an AABB that covers all LODs (though they would normally be the same size anyway).

Spatial does not have an AABB, VisualInstance cannot be created as its own object, and the other nodes that inherit VisualInstance had other functinoality such as lights, GIProbes, etc. I'd be open to a better implementation but this seemed like the best solution to me since it manages to be somewhat hidden as a LOD type anyway.

The signals are working quite well. I've been using them from the beginning without issue.

I tried your demo. Worked fine for me, as well.

What do you think about renaming LOD to MeshLOD to match the format of the others?

The thing about the main LOD object is that it's supposed to be a general container for a number of objects, but is able to work for meshes as well. This is also why it's ImmediateGeometry by default.

You could make a separate MeshLOD, but I'm not sure what kind of mesh-specific information you'd put there. Hence, a general LOD is used for the meshes. As a plus, you can include anything else you want there, as well, as well as groups of other objects.

After I worked on this update, I split up the changes into multiple commits so it's easier to see what the changes are. Some changes for a task may have been missed and pushed in subsequent commits.

No problem, thanks for keeping it organized 👍

With the current main tree, prior to this PR, when I debug the engine and quit a scene, I get this access violation upon closing. You can see in the call stack it is called from the LODManager destructor, which is empty. Do you know what might cause this? Even if I run LodManager.stop_loop() before closing, we get the same thing?

That's odd, I haven't seen that before.

It looks like an error trying to access the Semaphore in the destructor. The (potentially wrong) assumption here was that there isn't a scenario where the destructor runs without exit_tree() running and hence stop_loop(). Maybe it's trying to free it twice? Could be a problem with Godot? Maybe a long shot but I'd dry adding a null check to the Semaphore's free()... but if these changes somehow fixed it then I'm not sure... but the "fix" could be a coincidence. It might be good to add a null check there anyway (and check if that fixes it in the code before this PR to verify).

remove_object: Currently when changing scenes and potentially thousands of objects are exiting the tree, it searches through the arrays for every object. Potentially iterating over every array, every index, thousands of times. I'd rather just unregister it, but leave it in the array, then schedule a garbage collection to flush the arrays once.

Do you mean mark it unregistered in the LODManager? Registered in this context means it's part of the LODManager array.

That makes sense. I would just be very careful with preventing access to objects that aren't part of the tree or don't exist.

As for add_object, I'm thinking about pre counting the number of objects in _init, having lod_manager reserve the memory for the arrays. As it is, it pushes back each time, potentially rewriting the array thousands of times. I need to think about adding an object if it has been unregistered in enter_tree. I don't think it needs to be removed and readded to get it processing again. I think it just needs a tweak in lod_function

If it has a significant enough change in performance and you can think of a nice way to handle this I would be open to taking a look at that as well.

Also I need to test it more thoroughly with multiple scenes and many objects in my game.

Sounds good. It sounds like you have a lot more test cases than me as well as real world usage 👍

Looking good so far. A couple comments and I would just ask that you use more descriptive/longer names for the variables and also not use abbreviations (like lc).

And one question: What is the benefit over using vector instead of the Godot Array here?

Will review the rest later.

And one question: What is the benefit over using vector instead of the Godot Array here?

Basically, we don't need Godot to manage the memory for the array, it won't do it any faster, and so it doesn't even need to know about it.

Major plugins by other devs like Zylann use std::vector rather than Godot arrays.
Zylann's current recommendation is to use std::vector over Godot's arrays. a) bottom: https://github.com/godotengine/godot-cpp/issues/559#issuecomment-839753960 b) bottom: https://godotengine.org/qa/65086/custom-type-pool-array
Godot Arrays provide pool arrays for native data types, eg. PoolIntArray, which are optimized for native types. Custom types, would be a generic array of Variant::Object, for which there is no optimized pool type array.
The Godot array would be of type Variant::Object, whereas a C++ array template gives us an exact type of lbc*. Maybe this gives us type saftey in our code, and at runtime doesn't have to constantly cast objects back to Object 10,000 per frame.
The recommendation in 2019 was to use stl::vector, as Vector wasn't even exposed. https://github.com/godotengine/godot/issues/26300#issuecomment-467430738 Perhaps it's exposed now, but...
Godot's implementation isn't likely any better and probably worse than STL, even with the allegedly optimized pool arrays https://github.com/godotengine/godot-cpp/issues/680#issuecomment-1037875125
Vblanco found great performance benefit not using Godot data structures: For my fork i use STL and other containers from external libraries. I avoid godot containers as they are worse than the STL for performance, with the exception of the 2 hashmaps. https://github.com/godotengine/godot/issues/23998#issuecomment-512526009

Perfect, in that case sounds like a good idea!

From what I've seen your project has moved on to Godot 4, and I would assume it made this plugin obsolete for your purposes. However, I am considering porting this forward for Godot 4, and purposing it for flexible custom LODing/optimization similarly to the Unreal Engine Significance Manager.

Since this PR is a major amount of good work, what do you think about merging it in more or less its current feature state before porting it to Godot 4.0?

Sorry for not finishing. That was sloppy. I believe it was working fine, as we were using my version in Godot 3. Yes do whatever you like with it.

You can add my fork/branch as a remote and modify, rebase, or add commits before merging. I do it all the time on my Terrain3D submissions.

No problem. My support for this repo is pretty minimal at this point too, I just wanted to at least have it updated to Godot 4 and review any PRs that may come in.

Thanks for your contribution!

puchik / godot-extras

Add Signals and Improve processing #9