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 2D MultiMesh hierarchical culling #80106

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Aug 1, 2023

Fixes updating local bounds for MultiMeshes used in canvas items by introducing a back link.

This version as well as fixing CPUParticles2D, also fixes MultiMeshInstance2D and direct use of VisualServer.

Fixes #80086
Alternative to #80090

before

culling_broken.mp4

after

culling_fixed.mp4

This PR is basically an automated version of #80090 . canvas_item_invalidate_local_bound() is called automatically when changing the MultiMesh. This means users don't have to call this function.

The downside is that the machinery for maintaining the backlink is fairly complex, and complexity often equals bug prone and more maintenance burden. The system is equivalent to that used for Skeleton, which maintains a backlink to the canvas_item. However, the situation here is more complex for two reasons:

  1. MultiMesh references are stored in Item commands, rather than directly in the canvas_item. This means that adding / removing links has to to take place via the command list, rather than directly via the canvas_item. There can also potentially be multiple commands referencing the same MultiMesh. There is no need to reference count here, because commands are only cleared all at once, so there either is, or is not a reference held at any one time.
  2. Order of deletion is not guaranteed. We have to support both the MultiMesh being deleted before the canvas_item, and the other way around, with code to support both.

Designed to keep CanvasItem "light"

  • We could alternatively keep a LocalVector of linked MultiMeshes on the CanvasItem, which would in some ways be simpler (with no need to iterate through commands). However we want to keep CanvasItem as light as possible (most CanvasItems will be rects, and not care about MultiMesh), and storing the links in the commands is effectively "free", so it's probably worth the extra hassle.
  • Storing the canvas_item RID on each Command::MultiMesh is a little superfluous, but it enables us to clear out the links in the destructor with no cost to CanvasItems that do not contain multimeshes. The 8 extra bytes is likely to be unimportant, because most Command::MultiMeshes will only be stored singly on CanvasItems. Essentially it is more important to keep the typical CanvasItem light than the Command::MultiMesh.
    There may be a slightly better way of doing this (e.g. using a static or global to store the canvas_item RID during deletion) but storing the canvas_item RID is at least thread safe.

Notes

@lawnjelly lawnjelly requested a review from a team as a code owner August 1, 2023 08:11
@lawnjelly lawnjelly changed the title Fix 2D MultiMesh hierarchical culling [3.x] Fix 2D MultiMesh hierarchical culling Aug 1, 2023
@lawnjelly lawnjelly added this to the 3.6 milestone Aug 1, 2023
@lawnjelly lawnjelly marked this pull request as draft August 1, 2023 15:46
servers/visual/rasterizer.h Outdated Show resolved Hide resolved
@lawnjelly lawnjelly marked this pull request as ready for review August 1, 2023 15:58
Fixes updating local bounds for MultiMeshes used in canvas items by introducing a back link.
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes make sense, code looks good AFAICT.

I'd say don't merge without @clayjohn's review though.

@akien-mga akien-mga merged commit 39ed081 into godotengine:3.x Aug 8, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

I'd say don't merge without @clayjohn's review though.

Woops, I missed that one before merging. I'll ask Clay to review post-merge when he has time.

@clayjohn
Copy link
Member

clayjohn commented Aug 8, 2023

It looks fine from a visual review!

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.

4 participants