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

[Optimized] upload buffer #17595

Merged
merged 2 commits into from
Sep 3, 2024
Merged

[Optimized] upload buffer #17595

merged 2 commits into from
Sep 3, 2024

Conversation

GengineJS
Copy link
Contributor

@GengineJS GengineJS commented Sep 3, 2024

Greptile Summary

This pull request optimizes buffer management in the custom rendering pipeline, focusing on improving performance and reducing unnecessary updates. Key changes include:

  • Introduced currUniform property in DescBuffManager class for more efficient uniform handling
  • Implemented single buffer approach in DescBuffManager, simplifying buffer management
  • Added change detection before buffer updates to minimize unnecessary operations
  • Introduced idxRD parameter in several functions for improved indexing in buffer-related operations
  • Enhanced error handling and boundary checking in buffer-related functions for increased stability

These changes should lead to improved rendering performance by reducing buffer operations and optimizing memory usage.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Comment on lines 472 to 474
if (this.buffers.length === 1) {
return this.buffers[0];
}
Copy link
Contributor

@star-e star-e Sep 3, 2024

Choose a reason for hiding this comment

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

Why is this check needed? I think this.buffers is correctly resized in constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a determination of the array length, not the internal length of the buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean we can remove this guard, the rest of the code still works. I think this is an optimization, because when this.buffers.length === 1, we don't need to do the modulo?

Copy link

github-actions bot commented Sep 3, 2024

Interface Check Report

This pull request does not change any public interfaces !

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@star-e star-e merged commit 3ad291a into cocos:v3.8.4 Sep 3, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants