Replace Extents with Size in VoxelGI, ReflectionProbe, FogVolume, Decal and GPUParticles*3D

[Maran23]

I gave it a shot and revived the PR from @ator-dev.

Fixes: https://github.com/godotengine/godot/issues/54449 Supersedes: https://github.com/godotengine/godot/pull/55178 For more context see also: https://github.com/godotengine/godot/pull/44183 and https://github.com/godotengine/godot/pull/49672

Testing appreciated!

Co-authored-by: ator-dev [email protected]

[clayjohn]

I'm writing an early report of some issues I noticed while testing locally, I will provide more detailed comments on the code itself as well

When dragging a handle on the ReflectionProbe the editor popup is way off. It reports the extents instead of size. The extents are labelled "P", the position is labelled "S"

Similarly with FogVolumes "Size" reports the extents Actual size for comparison

FogVolume's gizmo is way off

I also noticed that this change introduces a shader compilation error into FogVolumes. I'll provide specific suggestions on the code momentarily.

[Maran23]

I'm writing an early report of some issues I noticed while testing locally, I will provide more detailed comments on the code itself as well

Thanks! I will take a look at it. I'm not super familiar with the 3D part of the engine, much more on the 2D part so this is super helpful!

[Maran23]

Update:

1. I fixed a lot of things I found.
2. I added Decal now as well, since the logic is very close to the rest.
3. I wonder if it makes sense to change the GPUParticles* nodes in another PR, so this PR does not grow much further? I have no strong opinion, I can also add them here. :)

[Calinou]

I wonder if it makes sense to change the GPUParticles* nodes in another PR, so this PR does not grow much further? I have no strong opinion, I can also add them here. :)

Yes, it makes sense to do this in the same PR.

[Maran23]

Yes, it makes sense to do this in the same PR.

Changed all the GPUParticles*3D as well.

[clayjohn]

@Maran23 We discussed this PR in our production meeting this morning. The consensus was that we would like to merge ithis ASAP as we don't want this work to go to waste and it is kind of now or never. However, we also want to minimize compatibility breakage (after all, we did promise to minimize compatibility breakage after Beta 1).

To minimize compatibility breakage, we need to add glue code to the _set/_get functions that can read when extents is used and automatically set size accordingly. I.e.:

#ifndef DISABLE_DEPRECATED
bool Decal::_set(const StringName &p_name, const Variant &p_value) {
	if (p_name == "extents") {
		set_size(p_value * 2.0);
		return true;
	}
	return false;
}

bool Decal::_get(const StringName &p_name, Variant &r_ret) const {
	if (p_name == "extents") {
		r_ret = get_size() / 2.0;
		return true;
	}
	return false;
}
#endif

Do you have time in the next 14 hours or so to add this? If not I will happily do it

[Maran23]

Do you have time in the next 14 hours or so to add this? If not I will happily do it

I will work on it right now. :)

[clayjohn]

Here is a test project built in Beta 16 that I will be using to check if extents are properly converted from save files extents-to-size.zip

Also tested GDScript code that sets .extents directly

[Maran23]

Here is a test project built in Beta 16 that I will be using to check if extents are properly converted from save files extents-to-size.zip

Also tested GDScript code that sets .extents directly

Thanks for the project. I opened it with Godot 4.beta16 and another copy with my branch. Looks good to me:

[akien-mga]

Thanks!