Skip to content
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

[3.x] Fix CPUParticles2D hierarchical culling #80090

Closed

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Jul 31, 2023

Updating the MultiMesh was previously not triggering a recalculation of local bounds in the hierarchical culling system. This PR adds an explicit function canvas_item_invalidate_local_bound() to trigger this update, and ensure particles are correctly culled.

This is called automatically by CPUParticles2D, but those users using 2D multimeshes via VisualServer directly will need to call canvas_item_invalidate_local_bound().

Fixes #80086
First commit is #80084 , that is useful for debugging and verifying the fix.

before

culling_broken.mp4

after

culling_fixed.mp4

Discussion

There are several ways of fixing this. It is possible to introduce a back link from the MultiMesh to the canvas item, and automatically invalidate the local bounds when it is updated. This is the system used for skeletons. However in this case the extra machinery seemed like overkill, and the same function could potentially be used to fix other cases of the same class of problem.

On the downside, this does mean that currently users updating 2D multimeshes directly to the VisualServer will be responsible for calling this function themselves, when using hierarchical culling.

I'm unsure which is best longterm, but this PR is nice and simple to get started, and may do the job, and we can always change to the other approach if feedback in a beta suggests it is needed.

GPUParticles2D

On further examination, GPU version only currently supports custom bounds, so there don't seem any changes needed.

Demo

particle_culling.zip

Just run it, the debug rect shows the local bounds correctly updating for the particles, even when the origin is offscreen.

Useful for debugging hierarchical culling.
@lawnjelly lawnjelly added this to the 3.6 milestone Jul 31, 2023
@lawnjelly lawnjelly requested review from a team as code owners July 31, 2023 15:45
@lawnjelly lawnjelly changed the title Fix CPUParticles2D hierarchical culling [3.x] Fix CPUParticles2D hierarchical culling Jul 31, 2023
@kleonc
Copy link
Member

kleonc commented Jul 31, 2023

On the downside, this does mean that currently users updating 2D multimeshes directly to the VisualServer will be responsible for calling this function themselves, when using hierarchical culling.

So if this is going to be merged as is then relevant docs (for multimeshes / hierarchical culling (does it have docs?) / something else?) would need to be updated to mention this, probably by adding some warning notes.

@lawnjelly
Copy link
Member Author

lawnjelly commented Aug 1, 2023

I'll also try an alternative PR which works similar to skeleton and produces a back link from the multimesh to the canvas_item. I'm not sure which would be better at this stage, but the latter has the advantage that it requires no changes from the user, even when using the servers directly.

There are no significant docs for hierarchical culling so far (other than the project setting itself), simply because it shouldn't require any, there's no changes required. Which would be kind of nice to keep. But if we go with this PR I can add a note to the docs for multimesh.

It's basically a choice between:

  • Little extra complexity in the engine
  • Responsibility of user to invalidate bounds for 2D multimesh

UPDATE:
Have completed the alternative PR. It does turn out to relatively complicated and not that elegant. There are pros and cons to both approaches so I will leave it to reviewers to decide. Personally I'm currently slightly more favouring this simpler version, but there is no perfect answer.

UPDATE 2:
Also, I've just noticed same problem will probably occur for MultiMeshInstance2D. #80106 should fix that automatically, but for this PR I'll have to include a back link from the MultiMesh node to the MultiMeshInstance2D scene side, so am making this draft again. If this turns out to be complex, then maybe #80106 may turn out to be a better bet.

Updating the MultiMesh was previously not triggering a recalculation of local bounds in the hierarchical culling system. This PR adds an explicit function `canvas_item_invalidate_local_bound()` to trigger this update, and ensure particles are correctly culled.
@lawnjelly lawnjelly force-pushed the hier_cull_invalidate_local_bound branch from e67ccf3 to 9ac4435 Compare August 1, 2023 08:25
@lawnjelly lawnjelly marked this pull request as draft August 1, 2023 08:54
@lawnjelly
Copy link
Member Author

Actually given all the permutations involved (with MultiMeshInstance2D) this could end up being as complex as fixing it server side in #80106 , and we are losing the main advantage of less complexity. So I'll close this PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants