Refactor FogVolume, VoxelGI, ReflectionProbe code #54796

ator-dev · 2021-11-09T10:25:56Z

ator-dev · 2021-11-09T10:55:42Z

I accidentally left an incomplete change in one file that invalidated the tests, could they be restarted please? I'm not familiar with this process.
Also, is force-pushing the right way to apply a fix to my pull request?

Calinou · 2021-11-09T11:03:34Z

I accidentally left an incomplete change in one file that invalidated the tests, could they be restarted please? I'm not familiar with this process.

Checks run automatically again whenever you (force-)push a commit to your branch.

Also, is force-pushing the right way to apply a fix to my pull request?

Yes, as it's the only way to ensure your PR continues to contain a single commit (rather than several) 🙂

ator-dev · 2021-11-09T11:05:21Z

I accidentally left an incomplete change in one file that invalidated the tests, could they be restarted please? I'm not familiar with this process.

Checks run automatically again whenever you (force-)push a commit to your branch.

Also, is force-pushing the right way to apply a fix to my pull request?

Yes, as it's the only way to ensure your PR continues to contain a single commit (rather than several) slightly_smiling_face

Thank you, that makes sense (;

ator-dev · 2021-11-09T14:01:40Z

@Calinou Since only one of the tests has failed, and that because of a documentation error, is it possible to accept the suggested changes and/or retry only the failed test? It seems like a waste to restart all of them again when only the failed one is affected by doc changes.

sairam4123 · 2021-11-09T14:23:32Z

Why was size chosen instead of extents though? Just wondering.

ator-dev · 2021-11-09T14:46:48Z

Why was size chosen instead of extents though? Just wondering.

See #54449 (probably there are other related issues as well) - in Godot 3 many of the 3D nodes with a concept of size (as a Vector3) utilised an extents property which was essentially half the actual size - so a MeshInstance with extents (1,2,1.5) took up 2×4×3 units of space. In Godot 4 the API and UI is being mostly standardised to use a size property instead, which is the actual size (or double the value of extents).
I'm not really refactoring apart from applying some of this standardisation now so that compatibility doesn't have to be broken later.
(Also, size does exist in a few places in 3.x and is used in exactly the same way.)

clayjohn

Looks mostly good! Just a few problems to iron out.

For future reference, it looks like there was agreement to expose size to users instead of extents but to use extents internally where it makes sense (Comment from Akien here)

clayjohn · 2021-11-09T17:08:53Z

drivers/gles3/shaders/scene.glsl

@@ -263,7 +263,7 @@ void light_compute(

 uniform highp mat4 refprobe1_local_matrix;
 out mediump vec4 refprobe1_reflection_normal_blend;
-uniform highp vec3 refprobe1_box_extents;
+uniform highp vec3 refprobe1_box_size;


Instead of converting the shader to use use size, you should convert from size to extents before sending to the shader.

That being said, this code is all unused right now and will be rewritten soon. So for now you can just revert all the changes to this file

clayjohn · 2021-11-09T17:22:50Z

servers/rendering/renderer_rd/renderer_scene_render_rd.cpp

@@ -5268,7 +5268,7 @@ void RendererSceneRenderRD::init() {
 			actions.renames["WORLD_POSITION"] = "world.xyz";
 			actions.renames["OBJECT_POSITION"] = "params.position";
 			actions.renames["UVW"] = "uvw";
-			actions.renames["EXTENTS"] = "params.extents";


You have changed the name of the parameter, but you didn't change the name in the actual shader (here). So this isn't going to compile. Further, above you are still passing the extents to the shader as you convert size into an extent in the line just above. You will also need to convert the CPU-side uniform to size if you are going to change the shader parameter (here)

Sorry, yeah - I had problems with the shader code, partly because I don't fully understand how it's called yet (and I also don't have error detection/squiggles for GLSL currently)

Would you mind pointing me towards where the values are passed into the relevant shaders please? I just can't seem to find it, so can't currently fix the input problem (I'm making the shaders use extents internally again).
By the way, should the visual shader expose extents (actually in the shader code) or size to the user?

The visual shader should use the same variable names as the text shader.

The CPU side variable is defined here:

godot/servers/rendering/renderer_rd/renderer_scene_render_rd.h

Line 762 in eb98fd9

float extents[3];

It is set here:

godot/servers/rendering/renderer_rd/renderer_scene_render_rd.cpp

Lines 3964 to 3966 in eb98fd9

volumetric_fog.push_constant.extents[0] = extents.x;

volumetric_fog.push_constant.extents[1] = extents.y;

volumetric_fog.push_constant.extents[2] = extents.z;

And in the shader it is defined here:

godot/servers/rendering/renderer_rd/shaders/volumetric_fog.glsl

Line 40 in eb98fd9

vec3 extents;

clayjohn · 2021-11-09T17:23:04Z

scene/resources/fog_material.cpp

@@ -158,7 +158,7 @@ uniform sampler3D density_texture: hint_white;
 void fog() {
    DENSITY = density * clamp(exp2(-height_falloff * (WORLD_POSITION.y - OBJECT_POSITION.y)), 0.0, 1.0);
    DENSITY *= texture(density_texture, UVW).r;
-    DENSITY *= pow(clamp(-SDF / min(min(EXTENTS.x, EXTENTS.y), EXTENTS.z), 0.0, 1.0), edge_fade);
+    DENSITY *= pow(clamp(-SDF / min(min(SIZE.x, SIZE.y), SIZE.z), 0.0, 1.0), edge_fade);


This is a change in logic as size is twice as big as extents.

ator-dev · 2021-11-09T18:13:46Z

Looks mostly good! Just a few problems to iron out.

For future reference, it looks like there was agreement to expose size to users instead of extents but to use extents internally where it makes sense (Comment from Akien here)

Thanks, I knew there'd probably be a few issues with my changes - I did my best to understand everything I was dealing with but had trouble with some of it (;
I'll start working on a fix soon.

ator-dev · 2021-11-11T10:46:07Z

I just realised that after using --doctool (thinking it would update the docs and so prevent the unit tests failing due to being outdated by my changes), the quality of the offline documentation has largely been decreased. I'll push a fix for this later; until then, don't worry about the newly missing bits, I wasn't actually intending to remove things like descriptions.

Refactor FogVolume code Fix FogVolume refactoring Refactor VoxelGI code Refactor ReflectionProbe code

ator-dev · 2021-11-20T18:26:14Z

@clayjohn Sorry, could you help me with the conflicts please? I don't really understand what's gone wrong, and can't seem to fix it.

clayjohn · 2021-11-20T22:04:40Z

It looks like in rebasing you have picked up changes from unrelated PRs. Can you undo the changes that are not relevant to this PR?

ator-dev · 2021-11-20T22:27:11Z

You're right... I'm just going to delete this branch and copy the relevant changes across. I've tried several times now to fix it and every time it somehow gets more convoluted.

Calinou · 2021-11-21T00:20:56Z

Superseded by #55178.

ator-dev requested review from a team as code owners November 9, 2021 10:25

ator-dev force-pushed the use-size-over-extents branch 2 times, most recently from 239390b to 3eeea74 Compare November 9, 2021 10:53

Calinou added breaks compat enhancement topic:3d topic:core labels Nov 9, 2021

Calinou added this to the 4.0 milestone Nov 9, 2021

ator-dev force-pushed the use-size-over-extents branch from 3eeea74 to fc81d66 Compare November 9, 2021 11:04

clayjohn requested changes Nov 9, 2021

View reviewed changes

ator-dev force-pushed the use-size-over-extents branch from e8410ad to c5e3215 Compare November 10, 2021 20:32

ator-dev requested review from clayjohn and removed request for a team November 10, 2021 20:33

ator-dev force-pushed the use-size-over-extents branch 3 times, most recently from 85f37c9 to 9c1ea56 Compare November 10, 2021 21:05

ator-dev requested review from a team as code owners November 10, 2021 21:05

ator-dev force-pushed the use-size-over-extents branch 3 times, most recently from 80c941a to 097254f Compare November 20, 2021 18:20

ator-dev requested review from a team as code owners November 20, 2021 18:20

ator-dev force-pushed the use-size-over-extents branch from 097254f to 21db9df Compare November 20, 2021 18:23

Refactor FogVolume, VoxelGI, ReflectionProbe code

9b28bce

Refactor FogVolume code Fix FogVolume refactoring Refactor VoxelGI code Refactor ReflectionProbe code

ator-dev force-pushed the use-size-over-extents branch from 21db9df to 9b28bce Compare November 20, 2021 18:25

ator-dev closed this Nov 21, 2021

ator-dev deleted the use-size-over-extents branch November 21, 2021 00:02

Calinou added the archived label Nov 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor FogVolume, VoxelGI, ReflectionProbe code #54796

Refactor FogVolume, VoxelGI, ReflectionProbe code #54796

ator-dev commented Nov 9, 2021

ator-dev commented Nov 9, 2021 •

edited

Loading

Calinou commented Nov 9, 2021 •

edited

Loading

ator-dev commented Nov 9, 2021

ator-dev commented Nov 9, 2021

sairam4123 commented Nov 9, 2021

ator-dev commented Nov 9, 2021 •

edited

Loading

clayjohn left a comment

clayjohn Nov 9, 2021

clayjohn Nov 9, 2021

ator-dev Nov 9, 2021

ator-dev Nov 9, 2021

clayjohn Nov 10, 2021

clayjohn Nov 9, 2021

ator-dev commented Nov 9, 2021

ator-dev commented Nov 11, 2021

ator-dev commented Nov 20, 2021

clayjohn commented Nov 20, 2021

ator-dev commented Nov 20, 2021

Calinou commented Nov 21, 2021

	volumetric_fog.push_constant.extents[0] = extents.x;
	volumetric_fog.push_constant.extents[1] = extents.y;
	volumetric_fog.push_constant.extents[2] = extents.z;

Refactor FogVolume, VoxelGI, ReflectionProbe code #54796

Refactor FogVolume, VoxelGI, ReflectionProbe code #54796

Conversation

ator-dev commented Nov 9, 2021

ator-dev commented Nov 9, 2021 • edited Loading

Calinou commented Nov 9, 2021 • edited Loading

ator-dev commented Nov 9, 2021

ator-dev commented Nov 9, 2021

sairam4123 commented Nov 9, 2021

ator-dev commented Nov 9, 2021 • edited Loading

clayjohn left a comment

Choose a reason for hiding this comment

clayjohn Nov 9, 2021

Choose a reason for hiding this comment

clayjohn Nov 9, 2021

Choose a reason for hiding this comment

ator-dev Nov 9, 2021

Choose a reason for hiding this comment

ator-dev Nov 9, 2021

Choose a reason for hiding this comment

clayjohn Nov 10, 2021

Choose a reason for hiding this comment

clayjohn Nov 9, 2021

Choose a reason for hiding this comment

ator-dev commented Nov 9, 2021

ator-dev commented Nov 11, 2021

ator-dev commented Nov 20, 2021

clayjohn commented Nov 20, 2021

ator-dev commented Nov 20, 2021

Calinou commented Nov 21, 2021

ator-dev commented Nov 9, 2021 •

edited

Loading

Calinou commented Nov 9, 2021 •

edited

Loading

ator-dev commented Nov 9, 2021 •

edited

Loading