Make TrackedRenderPass::set_vertex_buffer aware of slice size#14916
Merged
alice-i-cecile merged 4 commits intobevyengine:mainfrom Aug 28, 2024
Merged
Make TrackedRenderPass::set_vertex_buffer aware of slice size#14916alice-i-cecile merged 4 commits intobevyengine:mainfrom
alice-i-cecile merged 4 commits intobevyengine:mainfrom
Conversation
akimakinai
changed the title
alice-i-cecile
added
C-Bug
`DrawState` is not pub and used only in this file
akimakinai
force-pushed
the
fix_set_vertex_buffer
branch
from
b51d3df to
1508cf0
Compare
Contributor
Author
|
Rebased and removed workaround added in #14721. |
tychedelia
approved these changes
Aug 25, 2024
Member
tychedelia
left a comment
tychedelia
left a comment
There was a problem hiding this comment.
Looks good to me. Wish this code was easier to test. The previous assumption was obviously that an immutable slice itself can't change, but didn't anticipate slicing the same part of the buffer differently. Thanks for tracking this down!
|
|
||
| impl DrawState { | ||
| /// Marks the `pipeline` as bound. | ||
| pub fn set_pipeline(&mut self, pipeline: RenderPipelineId) { |
Member
There was a problem hiding this comment.
Just a note to other reviewers, it seems like this was a CI suggestion, and makes sense because DrawState is itself private to TrackedRenderPass.
bushrat011899
approved these changes
Aug 28, 2024
Contributor
bushrat011899
left a comment
bushrat011899
left a comment
There was a problem hiding this comment.
Looks good, just a small note around helper documentation.
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
tychedelia
added
S-Ready-For-Final-Review
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 30, 2025
…20468) # Objective - `offset` argument is misleading as the argument is not actually passed to wgpu (used only for memoization and logging). `BufferSlice` already contains an offset. - wgpu's `set_index_buffer` [sets the offset according to BufferSlice::offset](https://github.com/gfx-rs/wgpu/blob/e990388af98e4b4dff9f7fcc09a4eb5d2f71d227/wgpu/src/api/render_pass.rs#L98-L105) - `TrackedRenderPass::set_vertex_buffer` was made aware of slice size (#14916) but missed `set_index_buffer` counterpart ## Solution - Removed `offset` argument from `TrackedRenderPass::set_index_buffer` - Apply fix from #14916 to `TrackedRenderPass::is_index_buffer_set` - ~~Cleanup code by using the newly added `BufferSlice` getters~~ split out to #21289 ## Testing - Ran a few examples
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
Solution
TrackedRenderPassTesting
slicecomputes correct size thereMigration Guide
TrackedRenderPass::set_vertex_bufferfunction has been modified to update vertex buffers when the same buffer with the same offset is provided, but its size has changed. Some existing code may rely on the previous behavior, which did not update the vertex buffer in this scenario.