Skip to content

Mesh insert indices#11745

Merged
alice-i-cecile merged 6 commits intobevyengine:mainfrom
lynn-lumen:mesh_insert_attribute
Feb 6, 2024
Merged

Mesh insert indices#11745
alice-i-cecile merged 6 commits intobevyengine:mainfrom
lynn-lumen:mesh_insert_attribute

Conversation

@lynn-lumen
Copy link
Copy Markdown
Contributor

@lynn-lumen lynn-lumen commented Feb 6, 2024 •
edited
Loading

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.

@lynn-lumen
Fixes bevyengine#11740
58bb60c 
Added `Mesh::with_removed_indices` and updated `Mesh::set_indices` as well as `Mesh::with_indices`.
@lynn-lumen
Fixed typo
de87cde
@lynn-lumen
Copy link
Copy Markdown
Contributor Author

lynn-lumen commented Feb 6, 2024

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
Copy Markdown
Contributor

IceSentry commented Feb 6, 2024

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-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
Copy Markdown
Member

alice-i-cecile commented Feb 6, 2024

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

@mockersf
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

lynn-lumen commented Feb 6, 2024

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
@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

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

+1 more reviewer

@IceSentry IceSentry IceSentry approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change M-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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Turn Mesh::set_indices into Mesh::insert_indices

4 participants

@lynn-lumen @IceSentry @alice-i-cecile @mockersf