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 critical regressions introduced in PR #80414 #80552

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

DarioSamo
Copy link
Contributor

@DarioSamo DarioSamo commented Aug 12, 2023

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.

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.

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.
@DarioSamo
Copy link
Contributor Author

DarioSamo commented Aug 13, 2023

I've appended to this PR the other fix that was breaking CPU particles as well. Quoted from the commit.

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.

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.

@DarioSamo DarioSamo changed the title Fix incorrect error checking introduced in PR #80414. Fix critical regressions introduced in PR #80414 Aug 13, 2023
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.

Still looks good! This should be a low-risk merge

@akien-mga akien-mga merged commit 434d173 into godotengine:master Aug 14, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@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
Development

Successfully merging this pull request may close these issues.

4 participants