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

Faster exit from _cull_canvas_item if alpha is zero #85359

Merged

Conversation

miv391
Copy link
Contributor

@miv391 miv391 commented Nov 25, 2023

This is a tiny optimization for RendererCanvasCull::_cull_canvas_item(). There are a few checks in the beginning of that function which exit early if the item is not visible. Later in the function there is a check if modulate.a < 0.007 and if yes, that also means exit. Between the other checks and this alpha check some calculations are being made. Unless there are some very sneaky side effects, those calculations seem to be unnecessary if the modulate.a < 0.007 causes exit.

This PR moves modulate.a < 0.007 to be made right after other visibility checks.

My test project has 10000 sprites total, half of them (5000) have modulate.a = 0 and thus they are not visible. Results of my Visual Studio release build:

fps
master 310
this PR 340

I don't believe having a huge amount of sprites with alpha = 0 is a common use case. but this should not make any use case slower either.

@miv391 miv391 requested a review from a team as a code owner November 25, 2023 20:15
@YeldhamDev YeldhamDev added this to the 4.x milestone Nov 26, 2023
@akien-mga akien-mga changed the title Faster exit from _cull_canvas_item if alpha is zero Faster exit from _cull_canvas_item if alpha is zero Nov 26, 2023
@miv391
Copy link
Contributor Author

miv391 commented Nov 14, 2024

Anyone interested in reviewing this PR? The change is very small.

@clayjohn
Copy link
Member

The only concern I have is for the children nodes, Its possible for a node to have a module of 0, but for its children to be drawn. With this change, the children would not inherit the parent material or be sorted.

Perhaps the branches with side effects could also be moved up so they still happen before this check?

@miv391 miv391 force-pushed the faster-exit-from-cull_canvas_item branch from 4de49da to 18c1076 Compare November 15, 2024 17:07
@miv391
Copy link
Contributor Author

miv391 commented Nov 15, 2024

I moved all code with side effects to be executed before making the alpha check. Compared to the original code, only the material owner stuff and the alpha test itself was moved.

@miv391 miv391 force-pushed the faster-exit-from-cull_canvas_item branch from 18c1076 to 5c51101 Compare November 15, 2024 17:21
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

This makes sense and should be safe since the parts that have moved don't rely on the parts that now happen after the check.

The one thing that maaaaay cause issues is the line immediately below this change Rect2 rect = ci->get_rect(); as it updates the bounding rect for the canvas item. With this change the bounding rect will no longer be calculated when the item is hidden from having a low modulate.

I think this is fine because get_rect() caches the rect. So even if its not generated here, it is guaranteed to be generated before it is accessed.

I'm not suggesting it get moved for safety because I suspect that most of the performance benefit of this change comes from not re-calculating the rect. And for the above reasons I think it is safe not to call it in this situation.

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Nov 15, 2024
@@ -234,6 +234,12 @@ void RendererCanvasCull::_cull_canvas_item(Item *p_canvas_item, const Transform2
return;
}

Color modulate(ci->modulate.r * p_modulate.r, ci->modulate.g * p_modulate.g, ci->modulate.b * p_modulate.b, ci->modulate.a * p_modulate.a);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Color modulate(ci->modulate.r * p_modulate.r, ci->modulate.g * p_modulate.g, ci->modulate.b * p_modulate.b, ci->modulate.a * p_modulate.a);
Color modulate = ci->modulate * p_modulate;

There's already an operator for this, let's improve readability here while we're at it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@miv391 miv391 force-pushed the faster-exit-from-cull_canvas_item branch from 5c51101 to 39423d9 Compare November 16, 2024 11:27
@Repiteo Repiteo merged commit 3ded11d into godotengine:master Nov 18, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 18, 2024

Thanks!

@miv391 miv391 deleted the faster-exit-from-cull_canvas_item branch January 26, 2025 19:22
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.

6 participants