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

Mesh insert indices #11745

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Conversation

lynn-lumen
Copy link
Contributor

@lynn-lumen lynn-lumen commented Feb 6, 2024

Objective

Solution

  • Turned Mesh::set_indices into Mesh::insert_indices and added related methods for completeness.

Changelog

  • Replaced Mesh::set_indices(indices: Option<Indices>) with Mesh::insert_indices(indices: Indices)
  • Replaced Mesh::with_indices(indices: Option<Indices>) with Mesh::with_inserted_indices(indices: Indices) and Mesh::with_removed_indices() mirroring the API for inserting / removing attributes.
  • Updated the examples and internal uses of the APIs described above.

Migration Guide

  • Use Mesh::insert_indices or Mesh::with_inserted_indices instead of Mesh::set_indices / Mesh::with_indices.
  • If you have passed None to Mesh::set_indices or Mesh::with_indices you should use Mesh::remove_indices or Mesh::with_removed_indices instead.

Added `Mesh::with_removed_indices` and updated `Mesh::set_indices` as well as `Mesh::with_indices`.
@lynn-lumen
Copy link
Contributor Author

I'm not sure about the naming of Mesh::with_removed_indices. Would Mesh::without_indices be better?

@lynn-lumen lynn-lumen changed the title Mesh insert attribute Mesh insert indices Feb 6, 2024
@IceSentry
Copy link
Contributor

I personally prefer without_indices, but also, I'd prefer having with_indices and with_attribute. The _inserted_ part seems redundant to me.

But that's just personal preference, the current state of the PR LGTM

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Feb 6, 2024
@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 Feb 6, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 6, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 6, 2024
@alice-i-cecile
Copy link
Member

Looks like this needs a bit of clean up: CI errors look real.

@mockersf
Copy link
Member

mockersf commented Feb 6, 2024

new users of the renamed apis have been added to main, they need to be updated too

@lynn-lumen
Copy link
Contributor Author

new users of the renamed apis have been added to main, they need to be updated too

Should be good to go now

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 6, 2024
Merged via the queue into bevyengine:main with commit 4c86ad6 Feb 6, 2024
23 checks passed
@lynn-lumen lynn-lumen deleted the mesh_insert_attribute branch February 16, 2024 13:54
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-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

Turn Mesh::set_indices into Mesh::insert_indices
4 participants