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

flipping texture coords methods has been added to the StandardMaterial #12917

Merged

Conversation

bugsweeper
Copy link
Contributor

@bugsweeper bugsweeper commented Apr 10, 2024

Objective

Fixes #11996
The deprecated shape Quad's flip field role migrated to StandardMaterial's flip/flipped methods

Solution

flip/flipping methods of StandardMaterial is applicable to any mesh


Changelog

  • Added flip and flipped methods to the StandardMaterial implementation
  • Added FLIP_HORIZONTAL, FLIP_VERTICAL, FLIP_X, FLIP_Y, FLIP_Z constants

Migration Guide

Instead of using Quad::flip field, call flipped(true, false) method on the StandardMaterial instance when adding the mesh.

@bugsweeper bugsweeper force-pushed the add-flip-to-standardmaterial branch from 98fdc6f to e797807 Compare April 10, 2024 10:57
@bugsweeper
Copy link
Contributor Author

@alice-i-cecile May I ask you for a review?

@BD103 BD103 added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Apr 10, 2024
/// Flip the texture coordinates of the material.
pub fn flip(&mut self, horizontal: bool, vertical: bool) {
if horizontal {
self.uv_transform = Self::FLIP_HORIZONTAL * self.uv_transform;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.uv_transform = Self::FLIP_HORIZONTAL * self.uv_transform;
self.uv_transform *= Self::FLIP_HORIZONTAL;

Affine implements MulAssign, so you can do this instead.

Copy link
Contributor Author

@bugsweeper bugsweeper Apr 10, 2024

Choose a reason for hiding this comment

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

I think this replacement is not correct, because MulAssign will make equivalent of self.uv_transform = self.uv_transform * Self::FLIP_HORIZONTAL; but not self.uv_transform = Self::FLIP_HORIZONTAL * self.uv_transform;. The order is important for vector math. If somebody chain flipped() for StandardMaterial instance, he would expect that flipping transform would be done after original uv_transform, not before

Copy link
Member

Choose a reason for hiding this comment

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

I see. Could you add a comment explaining why the order is important, then? I could see this being confusing to others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

crates/bevy_pbr/src/pbr_material.rs Show resolved Hide resolved
crates/bevy_pbr/src/pbr_material.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile 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 Apr 10, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 10, 2024
Merged via the queue into bevyengine:main with commit ddcbb3c Apr 10, 2024
28 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-Feature A new feature, making something new possible 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.

Rectangle missing flip use case from deprecated shape Quad
4 participants