Reads from VisualServer causing unnecessary sync

[lawnjelly]

Godot version

3.5 stable, 4.0 alpha 13

System information

All

Issue description

This is just a reminder issue that we discovered today that some functions being called in VisualServer (gets) are potentially causing stalls in 3.x and 4.x. In general reading back data is discouraged except for the editor, and for one-off type operations like creating RIDs etc.

The example we found was #14971 which reduz says should not have been merged. We should instead be using something like call_deferred or VisualServerCallbacks to deferred return the result / signal without stalling.

The problematic function is:

virtual bool particles_get_emitting(RID p_particles) = 0;

The pattern of having something that could be read back every tick / frame is problematic. There may be several of these causing problems, e.g. also:

virtual bool particles_is_inactive(RID p_particles) = 0;
virtual AABB particles_get_current_aabb(RID p_particles) = 0;

There may be several of these we should convert to using deferred calls or alternative mechanisms where possible.

Steps to reproduce

Add a ParticleSystem2D with one shot mode, set to multithreaded renderer, and compile engine with #define DEBUG_SYNC.

Output is:

sync on: canvas_item_create
sync on: particles_create
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: shader_create
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
sync on: particles_get_emitting
etc

Minimal reproduction project

N/A

Discussion

These tend to be added in PRs by contributors who are not familiar with the command queue and separation between scene side and servers. Perhaps we could increase the documentation in the code in this area and have some policies on how to approach such problems.

[clayjohn]

This was fixed in 4.x by #76859, but a backport would be a good idea for 3.x

[matorin57]

I can look at backporting this to 3.x

[Calinou]

I can look at backporting this to 3.x

Feel free to open a pull request against the 3.x branch for this 🙂

[matorin57]

@Calinou Should be able to get one up pretty soon, but noticed that gpu_particles_3d.cpp/h and gpu_particles_2d.cpp/h don't exist on 3.x, meaning the back port will really just be deleting a string. Is this expected change we want?

Or do we want replace the 3.x particles.cpp/h with the new gpu versions?

[matorin57]

Oh I see there might be a mix up, do we want to backport #76859 or #76853?

[Calinou]

but noticed that gpu_particles_3d.cpp/h and gpu_particles_2d.cpp/h don't exist on 3.x,

These are called particles_2d.cpp and particles.cpp in 3.x respectively. (There was no GPU suffix back then, and many 3D nodes didn't have an explicit 3D suffix.)

[matorin57]

Sweet, moved the changes over by hand. Set up pr here: #81559

[zkcl]

Beginner here, want to help out. Is this issue fixed and just waiting to be reviewed in #81559?

[GumaD3v]

I see the issue is fixed both in 3.x and 4.x, shouldn't this issue be marked as completed?