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

[Merged by Bors] - Add Sprite Flipping #1407

Closed
wants to merge 1 commit into from

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Feb 5, 2021

OK, here's my attempt at sprite flipping. There are a couple of points that I need review/help on, but I think the UX is about ideal:

        .spawn(SpriteBundle {
            material: materials.add(texture_handle.into()),
            sprite: Sprite {
                // Flip the sprite along the x axis
                flip: SpriteFlip { x: true, y: false },
                ..Default::default()
            },
            ..Default::default()
        });

Now for the issues. The big issue is that for some reason, when flipping the UVs on the sprite, there is a light "bleeding" or whatever you call it where the UV tries to sample past the texture boundry and ends up clipping. This is only noticed when resizing the window, though. You can see a screenshot below.

image

I am quite baffled why the texture sampling is overrunning like it is and could use some guidance if anybody knows what might be wrong.

The other issue, which I just worked around, is that I had to remove the #[render_resources(from_self)] annotation from the Spritesheet because the SpriteFlip render resource wasn't being picked up properly in the shader when using it. I'm not sure what the cause of that was, but by removing the annotation and re-organizing the shader inputs accordingly the problem was fixed.

I'm not sure if this is the most efficient way to do this or if there is a better way, but I wanted to try it out if only for the learning experience. Let me know what you think!

@alice-i-cecile
Copy link
Member

In terms of UX, I think I have a preference for my_sprite.flip() over a field that you initialize / change / track. You would either want some way to specify the axis (an enum or maybe a line), or just have three convenience methods for each principal axis.

@cart
Copy link
Member

cart commented Feb 6, 2021

I don't think I agree. Imo it should just be a bool field for each axis. In most cases I have a feeling we'll either have

sprite.flipped = CONDITION_HERE;

or having the field explicitly set during construction.

If you truly don't care about the current state / just want to flip what's there, you can always do sprite.flipped = !sprite.flipped

More methods == more things to document / maintain / learn

@tigregalis
Copy link
Contributor

Can it be?:

pub struct Sprite {
    pub size: Vec2,
    pub flip_x: bool,
    pub flip_y: bool,
    #[render_resources(ignore)]
    pub resize_mode: SpriteResizeMode,
}

Or is the implementation too difficult/impossible?

@zicklag
Copy link
Member Author

zicklag commented Feb 6, 2021

@tigregalis it's definitely possible, but due to limitations of the current RenderResources derive macro we would have to manually implement RenderResources for both Sprite and TextureAtlasSprite, which would make the internal code messier. I'm good doing that if that's what @cart want's to do. It's mostly a matter of what we want to do from a code-style in the Bevy internals. ( or how much work it might be to somehow implement deriving RenderResources for struct with bool's in them, which might be an option, but would also be more work )

@cart
Copy link
Member

cart commented Feb 6, 2021

Haha i'll file this under "another case of RenderResources constraining our designs". I do like having flip_x and flip_y as siblings of size. Sprites are "core" enough that I think its worth the extra manual effort to build the interface we want. But lets add a // TODO: note to remove the manual impl when we finally sort out a permanent solution to RenderResources.

@zicklag
Copy link
Member Author

zicklag commented Feb 7, 2021

@cart Sounds good. I'll update this PR with the changes to move the fields, then. 👍

Copy link
Member Author

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

Pushed an updated moving the flip parameters adjacent to the other Sprite component parameters:

.spawn(SpriteBundle {
            material: materials.add(texture_handle.into()),
            sprite: Sprite {
                // Flip the logo to the left
                flip_x: true,
                ..Default::default()
            },
            ..Default::default()
        });

Comment on lines 24 to 38
// Flip the sprite if necessary
uint x_flip_bit = 1;
uint y_flip_bit = 2;
if ((flip & x_flip_bit) == x_flip_bit) {
uv = vec2(1.0 - uv.x , uv.y);
}
if ((flip & y_flip_bit) == y_flip_bit) {
uv = vec2(uv.x, 1.0 - uv.y);
}
Copy link
Member Author

@zicklag zicklag Feb 7, 2021

Choose a reason for hiding this comment

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

So here is where the rendering glitches are caused. If the UV's of the quad vertices truly are within the range of 0 to 1, then this subtraction we do to flip the UV ( 1.0 - uv.x ) should never result in a value below zero, and yet, when resizing the screen, we can see that the texture under/over-runs and samples outside of the texture image, resulting in clipping. I just don't know why.

It is worth noting that if I replace 1.0 with 0.9999, that it looks fine, but I'm not sure if it would look fine always, or if there would be other problems down the line. It just doesn't seem right when I feel like the other one should work.

Edit: I just realized that I think this is the same issue I had with my tilemap renderer, which was built on the same concept for how it laid out the tile UVs. It would run over the edge of the UVs when resizing the screen.

@MinerSebas
Copy link
Contributor

The new Example still needs to be added to the README.md in the example folder.

@zicklag
Copy link
Member Author

zicklag commented Feb 9, 2021

Updated!

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@zicklag zicklag force-pushed the sprite-flipping branch 3 times, most recently from 501ac73 to 8b4fb3d Compare February 27, 2021 18:19
@zicklag
Copy link
Member Author

zicklag commented Feb 27, 2021

I pushed an update that fixes the UV bleeding issues that remained, but I have no idea if my solution makes any real sense. My understanding of floating point arithmetic is limited at best, but my only guess as to the issue was that somehow there is a rounding issue where 1 - uv.x somehow ends up higher than it should be so I just subtracted the value of Rust's f32::EPSILON ( which again, I don't understand the mathematic foundation of ) to hopefully fix any rounding issues.

It did fix any issues that are visible while resizing the screen in the sprite_flipping example, so I think it's working. If nobody has any deeper knowledge about a better way to handle that, I think this is ready.

@cart
Copy link
Member

cart commented Mar 3, 2021

This looks good to go once we move to RenderResources derives.

@zicklag
Copy link
Member Author

zicklag commented Mar 3, 2021

There you go!

@cart
Copy link
Member

cart commented Mar 3, 2021

It looks like you changed Sprite back to a RenderResource derive. Was that intentional? Weirdly it still works for me, but I would expect it not to given the "2 bools merged into uint" behavior expected by the shader.

@cart
Copy link
Member

cart commented Mar 3, 2021

Ahhhh you changed the "bit". Thats interesting. Lemme think on that for a sec

@zicklag
Copy link
Member Author

zicklag commented Mar 3, 2021

Oh, now that you say that I get what you meant in your above comment. I thought you were saying that's what I should do, but you were just talking about RenderResources, not RenderResource. I had stumbled upon why the y flipping wasn't working when I switched it back to the derive and noticed that each bool was taking a whole byte and not one bit.

Because of padding bytes and alignment we don't take up any more bytes of data I don't think.

Also, do we need the Sprite struct to be #[repr(C)] to guarantee soundly implementing Bytable?

@cart
Copy link
Member

cart commented Mar 3, 2021

(not accounting for alignment/padding) the RenderResource/Byteable impl would result in a "slightly too large" buffer as Byteable has no way to ignore resize_mode. Can we move back to a manual impl for now (both for consistency with TextureAtlasSprite and clarity on dataflow/type).

Also, do we need the Sprite struct to be #[repr(C)] to guarantee soundly implementing Bytable?

Yeah for future reference #[repr(C)] is required.

@zicklag
Copy link
Member Author

zicklag commented Mar 3, 2021

Ah, yes. Give me one second.

@zicklag
Copy link
Member Author

zicklag commented Mar 3, 2021

There we go.

@cart
Copy link
Member

cart commented Mar 3, 2021

bors r+

bors bot pushed a commit that referenced this pull request Mar 3, 2021
OK, here's my attempt at sprite flipping. There are a couple of points that I need review/help on, but I think the UX is about ideal:

```rust
        .spawn(SpriteBundle {
            material: materials.add(texture_handle.into()),
            sprite: Sprite {
                // Flip the sprite along the x axis
                flip: SpriteFlip { x: true, y: false },
                ..Default::default()
            },
            ..Default::default()
        });
```

Now for the issues. The big issue is that for some reason, when flipping the UVs on the sprite, there is a light "bleeding" or whatever you call it where the UV tries to sample past the texture boundry and ends up clipping. This is only noticed when resizing the window, though. You can see a screenshot below.

![image](https://user-images.githubusercontent.com/25393315/107098172-397aaa00-67d4-11eb-8e02-c90c820cd70e.png)

I am quite baffled why the texture sampling is overrunning like it is and could use some guidance if anybody knows what might be wrong.

The other issue, which I just worked around, is that I had to remove the `#[render_resources(from_self)]` annotation from the Spritesheet because the `SpriteFlip` render resource wasn't being picked up properly in the shader when using it. I'm not sure what the cause of that was, but by removing the annotation and re-organizing the shader inputs accordingly the problem was fixed.

I'm not sure if this is the most efficient way to do this or if there is a better way, but I wanted to try it out if only for the learning experience. Let me know what you think!
@bors
Copy link
Contributor

bors bot commented Mar 3, 2021

@bors bors bot changed the title Add Sprite Flipping [Merged by Bors] - Add Sprite Flipping Mar 3, 2021
@bors bors bot closed this Mar 3, 2021
@zicklag zicklag deleted the sprite-flipping branch March 3, 2021 19:44
bors bot pushed a commit that referenced this pull request Mar 7, 2021
Since 8921717, some birds in example `contributors` where not colored.

Fix is to use `flip_x` of `Sprite` instead of setting `transform.scale.x` to `-1` as described in #1407.


It may be an unintended side effect, as now we can't easily display a colored sprite while changing it's scale from `1` to `-1`, we would have to change it's scale from `1` to `0`, then flip it, then change scale from `0` to `1`.
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.

5 participants