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

Added Vec append to BufferVec - Issue #3531 #8575

Merged

Conversation

Connor-McMillin
Copy link
Contributor

Objective

Solution

  • Added an append wrapper to BufferVec based on the function signature for vec.append()

First PR to Bevy. I didn't see any tests for other BufferVec methods (could have missed them) and currently this method is not used anywhere in the project. Let me know if there are tests to add or if I should find somewhere to use append so it is not dead code. The issue mentions implementing truncate and extend which were already implemented and merged here

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels May 9, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 9, 2023

I would like to see a couple of simple tests for this: seems like useful functionality both internally and externally but I agree that dead code is rough.

@Connor-McMillin
Copy link
Contributor Author

Are there any other vector functions that should get added with this PR?

@Connor-McMillin
Copy link
Contributor Author

Connor-McMillin commented May 9, 2023

Thanks @mockersf for pointing out that I needed to make the function public. This also resolved the dead code warnings, although I don't know why it doesn't detect this as dead code anymore.

The tests I wrote (and momentarily pushed to this branch)

  • Didn't resolve the dead code warning
  • Could not easily use a test struct that derives Pod because that requires a dependency on bytemuck which bevy_render doesn't require currently and requiring that dependency for one or two tests doesn't seem like a good idea
  • Were pretty gross using Mat4 and matrix operations in assertions. This was one of the objects I found that derives Pod and didn't require additional dependencies to other projects

So I think just the implementation sans tests probably makes the most sense and is in line with the rest of BufferVec not being tested directly

@mockersf
Copy link
Member

mockersf commented May 9, 2023

Thanks @mockersf for pointing out that I needed to make the function public. This also resolved the dead code warnings, although I don't know why it doesn't detect this as dead code anymore.

Because it's now part of the exposed public API, so it can be used by caller of the lib

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 9, 2023
@james7132 james7132 added A-Rendering Drawing game state to the screen and removed A-ECS Entities, components, systems, and events labels May 9, 2023
@james7132 james7132 added this pull request to the merge queue May 9, 2023
Merged via the queue into bevyengine:main with commit f786ad4 May 9, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose more of the Vec API on BufferVec
4 participants