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

Fix integer underflow when rounding up in VoxelGI #80356

Merged

Conversation

darksylinc
Copy link
Contributor

The code wanted to divide and round up:

  • 0 / 64 = 0
  • 63 / 64 = 1
  • 64 / 64 = 1
  • 65 / 64 = 2

However when the dividend was exactly 0 it would underflow and produce 67108864 instead.

This caused TDRs on empty scenes or extremely slow performance

Fix #80286

The code wanted to divide and round up:
 - 0 / 64 = 0
 - 63 / 64 = 1
 - 64 / 64 = 1
 - 65 / 64 = 2

However when the dividend was exactly 0 it would underflow and produce
67108864 instead.

This caused TDRs on empty scenes or extremely slow performance

Fix godotengine#80286
@lawnjelly
Copy link
Member

Given that this is used in multiple places, and this error has creeped in (probably via copy paste), would this be better to fix once and use a macro or inlined math function in the different uses? 🤔

e.g. #define GODOT_ROUND_UP(N, S) ((((N) + (S) - 1) / (S)) * (S))

(and / or version without the multiplier as you are using here)

@Calinou Calinou added bug topic:rendering topic:3d cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 7, 2023
@Calinou Calinou added this to the 4.2 milestone Aug 7, 2023
@Calinou
Copy link
Member

Calinou commented Aug 7, 2023

Note that doesn't appear to resolve #55359. (I tested this as requested in #80286 (comment).)

OS: Linux
CPU: Intel Core i9-13900K
GPU: GeForce RTX 4090
Resolution: 3840×2160

All tests done with VoxelGI quality set to High in the project settings.

Mode Before (master) After (this PR)
Dynamic + Static 358 FPS (2.79 mspf) 355 FPS (2.81 mspf)
Dynamic only 359 FPS (2.78 mspf) 356 FPS (2.80 mspf)
Static only 524 FPS (1.90 mspf) 523 FPS (1.91 mspf)
Neither dynamic nor static 258 FPS (1.89 mspf) 528 FPS (1.89 mspf)

@AThousandShips
Copy link
Member

I believe you have a transposition error in row four column one, 258 instead of 528

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.

Looks great!

@akien-mga akien-mga merged commit 8a3a3aa into godotengine:master Aug 7, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants