-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 critical regressions introduced in PR #80414 #80552
Fix critical regressions introduced in PR #80414 #80552
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I missed this too! Looks good.
…engine#80414. There was an error in the other branch of the refactored function where the size of the array was not properly multiplied by the size of the float to check against the buffer size. This was only an error in the error-checking itself and not the functionality. There was also an error where the proper notification was not emitted whenever the buffer for the multimesh is recreated to invalidate the previous references the renderer might've created to it. This fixes CPU Particles getting corrupted when they're created without emission being enabled.
1a7d0f3
to
420f389
Compare
I've appended to this PR the other fix that was breaking CPU particles as well. Quoted from the commit.
Adding the following lines to the end of the function did the trick. // Invalidate any references to the buffer that was released and the uniform set that was pointing to it.
multimesh->dependency.changed_notify(Dependency::DEPENDENCY_CHANGED_MULTIMESH); The change makes a lot of sense due to there being a time gap between the time the render instance can be created and its buffer being modified, but the renderer retains a reference to the old buffer. The proper way to clear that is through the notification system in this case. I've tested it on both projects (the MRP and Platformer with TAA) and it works fine, but we should re-review the changes anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good! This should be a low-risk merge
Thanks! |
Cherry-picked for 4.1.2. |
Glad to share my first PR for fixing an error I introduced myself in another PR (#80414) that I somehow got past the review. 🎉
There was an error in the other branch of the refactored function where the size of the array was not properly multiplied by the size of the float to check against the buffer size. This was only an error in the error-checking itself and not the functionality.
The error was easily reproduced by enabling TAA on the 3D Platformer project and seeing the terrain disappear. There's something in particular that makes use of the data cache in the project, which triggered the other branch and caused the incorrect error to show up.
Upon review it should be evident the size of the array needs to be multiplied by
sizeof(float)
to actually match the buffer size on the error-check itself.