-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CPU/GPUParticles2D bugs on Compatibility Rendering (GLES3) on Adreno 3XX devices. #88816
Fix CPU/GPUParticles2D bugs on Compatibility Rendering (GLES3) on Adreno 3XX devices. #88816
Conversation
55762de
to
53a61b0
Compare
@clayjohn can you review it, please? |
a975be1
to
b257686
Compare
@Calinou do you think this PR can be cherry-picked for the 4.2.2 release? |
@joined72 I expect that the particles process material would be more problematic than actually drawing the particles as drawing particles works the same way as drawing CPU particles or drawing MultiMeshes. Have you tested using CPUParticles2D instead of GPUParticles2D? It would be helpful to know if CPUParticles2D causes the same crash |
@clayjohn CPUParticles2D don't cause any crash, but still not be showed and don't decrease FPS, so I focused only on GPUParticles2D. EDIT: if you think I can focus only on the |
Let me Explain in details... The freeze was caused by an incompatibility in the Fixed that, I found that the Fixed that I thought to see the While I was working on showing the particles, I understand that the device is too old and slow to handle particles smoothly, so I decided to disable At the start, I done this working on the PS: from my test |
Thank you for clarifying! That makes sense to me now. So it seems the problem is that MultiMeshes are not drawing correctly in the 2D renderer. I don't think that is a limitation of Adreno 3XX though. It is more likely a problem with our code since regular rect drawing uses a similar pathway to MultiMeshes. I'm assuming you are testing on master in which case, you could actually be seeing a recently introduced issue that was fixed yesterday by #88938. Could you test with CPUParticles again on the most recent master branch and see if they are visible now? To be clear, I think we should investigate why particles are not appearing instead of disabling them on Adreno devices. |
Ok, I'll update ASAP.
Great 😃 |
@clayjohn I'm sorry but the most recent master branch has the same issue, Based on my experience, the problem "should" be using some form of Shader that uses GLSL code incompatible with Adreno 3XX but compatible with all other devices. I tried checking in the Could you give me some directions? EDIT: is possible the Adreno 3XX Shader "incompatibility" is in the Shader generated by the
|
Based on your description, the only shader that could be causing this issue is https://github.com/godotengine/godot/blob/master/drivers/gles3/shaders/canvas.glsl Particularly with the If the MeshInstance2D does not appear, then the issue is with If the MultiMeshInstance2D does appear correctly. Then the issue is most likely with using both custom color and custom data. So try enabling those on the MultiMeshInstance2D. CPUParticles internally are just a MultiMeshInstance2D with both custom color and custom data enabled. |
On a side note, in the rendering meeting we discussed if there would be a performance impact to using an if/else if branch instead of a switch statement. I took a look and confirmed that on AMD the generated bytecode is the same between the two approaches: Switch: https://shader-playground.timjones.io/6145551406931ced2e87820428c9e21f |
Really Great, thanks for the update! ;) PS: I'm working on the UPDATE: Logcat...
The problem is "Unable to initialize engine render view" at GodotFragment.java:196
I'm working on the 4.2.1-stable branch now, to avoid any other extra issues. |
Problem solved, was caused from my wrong configuration! About the |
I found other useful info... the problem is in this part of the drivers/gles3/shaders/canvas.glsl sources: The following line: godot/drivers/gles3/shaders/canvas.glsl Line 181 in dad6c77
returns ALWAYS I'm continuing my investigation... |
Other info... FROM: vec4 instance_color = vec4(unpackHalf2x16(instance_color_custom_data.x), unpackHalf2x16(instance_color_custom_data.y));
color *= instance_color; TO: vec4 instance_color = vec4(unpackHalf2x16(instance_color_custom_data.x), unpackHalf2x16(instance_color_custom_data.y));
instance_color.w = 1.0;
instance_color.z = 1.0;
color *= instance_color; I force BLUE and ALPHA channels ALWAYS to 100% but I CAN SEE CPUParticles2D, but ONLY if I use a NOT empty |
GREAT NEWS! ;) I found a solution. Changing the following drivers/gles3/shaders/canvas.glsl source code: FROM: if (bool(read_draw_data_flags & FLAGS_INSTANCING_HAS_COLORS)) {
vec4 instance_color = vec4(unpackHalf2x16(instance_color_custom_data.x), unpackHalf2x16(instance_color_custom_data.y));
color *= instance_color;
}
if (bool(read_draw_data_flags & FLAGS_INSTANCING_HAS_CUSTOM_DATA)) {
instance_custom = vec4(unpackHalf2x16(instance_color_custom_data.z), unpackHalf2x16(instance_color_custom_data.w));
} TO: if (bool(read_draw_data_flags & FLAGS_INSTANCING_HAS_COLORS)) {
vec4 instance_color;
instance_color.xy = unpackHalf2x16(instance_color_custom_data.x);
instance_color.zw = unpackHalf2x16(instance_color_custom_data.y);
color *= instance_color;
}
if (bool(read_draw_data_flags & FLAGS_INSTANCING_HAS_CUSTOM_DATA)) {
instance_custom.xy = unpackHalf2x16(instance_color_custom_data.z);
instance_custom.zw = unpackHalf2x16(instance_color_custom_data.w);
} FIX ANY @clayjohn what do you think about? If you think these are acceptable changes then the following piece of code would also need to be modified, although I don't know exactly what it does. if (gl_VertexID % 3 == 0) {
vertex = read_draw_data_point_a;
uv = read_draw_data_uv_a;
color = vec4(unpackHalf2x16(read_draw_data_color_a_rg), unpackHalf2x16(read_draw_data_color_a_ba));
} else if (gl_VertexID % 3 == 1) {
vertex = read_draw_data_point_b;
uv = read_draw_data_uv_b;
color = vec4(unpackHalf2x16(read_draw_data_color_b_rg), unpackHalf2x16(read_draw_data_color_b_ba));
} else {
vertex = read_draw_data_point_c;
uv = read_draw_data_uv_c;
color = vec4(unpackHalf2x16(read_draw_data_color_c_rg), unpackHalf2x16(read_draw_data_color_c_ba));
}
NOTE: all these changes did not fix the |
@clayjohn from my research, it seems that the problem is that the drivers/gles3/shaders/particles_copy.glsl shader, receives incorrect parameters (specifically // if (bool(floatBitsToUint(velocity_flags.w) & PARTICLE_FLAG_ACTIVE))
I tried to figure out who passed the parameters to particles_copy.glsl shader, but I couldn't. I thought it was passed to it by the particles.glsl, but by changing its |
@clayjohn particles.glsl - flags = floatBitsToUint(velocity_flags.w);
+ flags = PARTICLE_FLAG_ACTIVE; // On Adreno 3XX 'velocity_flags.w' is ALWAYS ZERO!
...
- out_velocity_flags.w = uintBitsToFloat(flags);
+ out_velocity_flags.w = float(flags); particles_copy.glsl - if (bool(floatBitsToUint(velocity_flags.w) & PARTICLE_FLAG_ACTIVE)) {
+ if (bool(uint(velocity_flags.w) & PARTICLE_FLAG_ACTIVE)) {` What do you think about? I have testes these changes on NOT Adreno 3XX devices and works fine (also forcing |
From the Chat...
@clayjohn I understand that If I could understand this well, I think I could understand why, in Adreno 3XX devices, this parameter is always ZERO. EDIT: I think is possible that the particles.glsl EDIT2: I think to have found the line here: glEnableVertexAttribArray(1); // .xyz: velocity. .z: flags.
glVertexAttribPointer(1, 4, GL_FLOAT, GL_FALSE, stride, CAST_INT_TO_UCHAR_PTR(stride * lifetime_split + sizeof(float) * 4 * 1));
I'm working on it ;) |
98fe6c9
to
254438a
Compare
A few days ago I received a Sony Xperia M2 (Adreno 305), which seems to be one of the oldest devices in the Adreno 3XX family. I bought it to check if my fixes were sufficient even for such an old device and I discovered that they were NOT. I therefore had to do more research to get What I discovered is that such old Adreno drivers have the following extra bugs:
To fix all these bugs I had to rename all the functions of stdlib_inc.glsl shader (adding the @clayjohn can you take a look and tell me what do you think, please? |
I'm running some benchmarks to test the new From my tests, results that calling the Test results (100.000 calls/secs):
There is a really minimal performance loss only on very old devices but we have to consider that we are calling these functions 100.000 times/sec. I think the driver implementation is very similar to the |
1f3ca0e
to
e7176fb
Compare
Currently this PR brings the following fixes:
Considering that the number of bugs to fix Can I ask for the review required for the merge, please? ;) |
e7176fb
to
f8ca4a9
Compare
f3f11b4
to
4df39dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now! Great investigative work
Thanks! |
Fixes #88815
This PR is the natural completion of #87352
Tested Devices:
(Adreno 305)
- FIXED(Adreno 308)
- FIXED(Adreno 320)
- FIXED(Adreno 330)
- FIXED(Adreno 420)
- NOT AFFECTED(Adreno 505)
- NOT AFFECTED(Adreno 530)
- NOT AFFECTED(Adreno 610)
- NOT AFFECTED(Mali T830)
- NOT AFFECTEDThis PR allows a game that uses
GPUParticles2D
on older Adreno 3XX devices to start and run fine (without any freeze) but not allow to useGPUParticles2D
(however, it allows you to fully use theCPUParticles2D
, which otherwise would not work at all)...Let me Explain...
After fixed all the incompatibilities of the Particle Shaders that caused the game to freeze at startup (only on Adreno 3XX devices), I saw that the particles were still NOT displayed even though the FPS drop showed that they were still processed.
The drop in FPS was truly notable, even just inserting an "empty"
GPUParticles2D
Node, there was a drop of 11/13 FPS.At this point I completely disabled the
GPUParticles2D
functionality (only on Adreno 3XX devices) because trying to go further no longer made sense since these devices are too old and slow to be able to handle particles smoothly.So the scope of this PR is ONLY let a game with
GPUParticles2D
start and run fine (without Freeze) on old Adreno 3XX devices (but without any particles).Screenshot of a demo game on a LG Nexus 5 (Adreno 330) before the fixes...
After the fixes... (you cannot see the Particles because was disabled, but at least the game is playable)