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

Rectangle missing flip use case from deprecated shape Quad #11996

Closed
tripokey opened this issue Feb 20, 2024 · 7 comments · Fixed by #12917
Closed

Rectangle missing flip use case from deprecated shape Quad #11996

tripokey opened this issue Feb 20, 2024 · 7 comments · Fixed by #12917
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use P-Regression Functionality that used to work but no longer does. Add a test for this!
Milestone

Comments

@tripokey
Copy link
Contributor

What problem does this solve or what need does it fill?

The deprecated shape Quad's flip field does not have a clear migration path to the new Rectangle shape.

Current migration guide: https://bevyengine.org/learn/migration-guides/0-12-to-0-13/#deprecate-shapes-in-bevy-render-mesh-shape

What solution would you like?

#10569 specifies that there should be a flipped method to cover this use case, however that seems to be missing.

let flipped_rectangle = Rectangle::new(100.0, 100.0).mesh().flipped();
@tripokey tripokey added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Feb 20, 2024
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Feb 20, 2024
@alice-i-cecile
Copy link
Member

Ah, this is to invert the normals, correct? Should we add this as an option on all of our mesh builders?

@alice-i-cecile alice-i-cecile added A-Math Fundamental domain-agnostic mathematical operations C-Usability A targeted quality-of-life change that makes Bevy easier to use P-Regression Functionality that used to work but no longer does. Add a test for this! and removed C-Feature A new feature, making something new possible labels Feb 20, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Feb 20, 2024
@tripokey
Copy link
Contributor Author

It flipped the texture coordinates horizontally.

From https://docs.rs/bevy/latest/bevy/prelude/shape/struct.Quad.html :

Horizontally-flip the texture coordinates of the resulting mesh

@Jondolf
Copy link
Contributor

Jondolf commented Feb 20, 2024

This was sort of left out on purpose: #11431 (comment)

It seems a bit strange to me to support flipping a texture in a mesh representation. I feel like it's something that should instead be configurable at a material-level. I would assume that this is possible already? If not, I think it'd be valuable to have.

Only supporting texture flipping for Rectangle/Quad also seems slightly arbitrary to me; why not also support planes? Or circles? Or basically any shape?

Looking at other applications, I'm also not seeing a similar flip property elsewhere. For example, Godot doesn't have it (I also checked editor GUI), Unity doesn't have it, and Blender doesn't have it. To me, it doesn't feel like a quad-specific property, but a general material property.

@tripokey
Copy link
Contributor Author

I'm not saying that the flipped method needs to be added to Mesh, as a user of Quad::flip I think that a line in the migration guide on how to achieve this in the new bevy release would be enough

@bugsweeper
Copy link
Contributor

bugsweeper commented Mar 28, 2024

I feel like it's something that should instead be configurable at a material-level

Do you mean just add to StandardMaterial additional flip parameter, or mention in migration guide that making custom flipping material is the correct way for replacement of Quad::flip?

@bugsweeper
Copy link
Contributor

bugsweeper commented Apr 9, 2024

@tripokey For any UV transformations you can use StandardMaterial::uv_transform, which is pretty fresh, because it had been added for KHR_texture_transform support to current main and it will be available from next 0.14 release.
For example for flip you can try set uv_transform to
Affine2{matrix2: -Mat2::IDENTITY, translation: Vec2::ONE,} for both axes flip
or

Affine2 {
    matrix2: Mat2::from_cols(-Vec2::X, Vec2::Y),
    translation: Vec2::X,
} // for horizontal flip

instead of default uv_transform or premultiply it with uv_transform that you wanted to use earlier, if so.
@alice-i-cecile How do you think, should we add flipped method to StandardMaterial, or maybe add FLIPPING / HORIZONTAL_FLIPPING const value to Affine2 consts? Or just copy valuable part of this message to migration log?

@alice-i-cecile
Copy link
Member

I think that StandardMaterial is the most predictable place to find this. I think we should do that, but also add some nice constants to Affine2 (and possibly Affine3): those are great simple constants that will come up repeatedly and improve readability.

github-merge-queue bot pushed a commit that referenced this issue Apr 10, 2024
#12917)

# 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.

---------

Co-authored-by: BD103 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use P-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants