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

Thin line in 9-sliced texture when sprite resolution is high #14183

Closed
Litttlefish opened this issue Jul 6, 2024 · 11 comments · Fixed by #14990
Closed

Thin line in 9-sliced texture when sprite resolution is high #14183

Litttlefish opened this issue Jul 6, 2024 · 11 comments · Fixed by #14990
Assignees
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Regression Functionality that used to work but no longer does. Add a test for this!
Milestone

Comments

@Litttlefish
Copy link
Contributor

Bevy version

0.14

What you did

Update to 0.14

What went wrong

图片
As the picture shown, there are thin lines in the background.

Additional information

The original picture resolution is 4x larger than the sprite, I don't know if this is the reason

@Litttlefish Litttlefish added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jul 6, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14.1 milestone Jul 6, 2024
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels Jul 6, 2024
@extrawurst
Copy link
Contributor

Running into the same issue. @Litttlefish did you find a workaround for it?

@Litttlefish
Copy link
Contributor Author

Running into the same issue. @Litttlefish did you find a workaround for it?

nope, no idea why this happens

@marcelchampagne
Copy link
Contributor

Also running into the exact same issue after upgrading to 0.14.0. Smaller buttons using 9-slices have the lines as shown in the image.

@janhohenheim janhohenheim added the C-Regression Functionality that used to work but no longer does. Add a test for this! label Jul 12, 2024
@MScottMcBee
Copy link
Contributor

MScottMcBee commented Jul 20, 2024

Bisecting the issue, I think it's caused by the shader changes from #13523. It's reproducible in the ui_texture_slice example if you add with_scale_factor_override(4.0)

Digging in more it looks like the fact I'm using a scale factor if 4 is important. It doesn't happen if I use 1 or 2. Finding #13814, I changed the aliasing in my local branch to be return clamp(0.0, 1.0, 0.5 - 4.0 * distance); and it fixed the issue for me.

I'm wondering if we could have AA be a flag or something in that shader. My game has pixel art so I want to turn all AA off.

@alice-i-cecile alice-i-cecile modified the milestones: 0.14.1, 0.14.2 Aug 2, 2024
@Jaso333
Copy link

Jaso333 commented Aug 3, 2024

I observe this in the examples rendered in WebGL on the bevyengine website:

image

At default UiScale, I also observe this with my own 9patch button on 0.14.1. It is even worse at higher UI scales.

@StarArawn
Copy link
Contributor

I noticed this same issue but worse in a non UI or sprite renderer. I noticed the resulting slices from the slicer can have a lot of precision this can easily result in errors like this.

@Jaso333
Copy link

Jaso333 commented Aug 4, 2024

Bisecting the issue, I think it's caused by the shader changes from #13523. It's reproducible in the ui_texture_slice example if you add with_scale_factor_override(4.0)

Digging in more it looks like the fact I'm using a scale factor if 4 is important. It doesn't happen if I use 1 or 2. Finding #13814, I changed the aliasing in my local branch to be return clamp(0.0, 1.0, 0.5 - 4.0 * distance); and it fixed the issue for me.

I'm wondering if we could have AA be a flag or something in that shader. My game has pixel art so I want to turn all AA off.

I did the same thing, except I changed the "0.5" to "1.0", as I think you need to double all terms in this calculation to change the AA threshold on the distance. This seemed to fix the problem, but I don't really know why. Maybe this change effectively disabled the anti-aliasing.

To highlight the change at UI scale factor of 8:

with "0.5" and "2.0" (the original values):
image

with "1.0" and "4.0" (this is precisely how my button should look):
image

I agree with @MScottMcBee, anti-aliasing is hard-coded to be enabled in bevy_ui, but probably shouldn't be? However, this form of anti-aliasing doesn't seem to be effective anyway, as the artifacts show for any scale or art style (not just pixel-art), so maybe it just needs correcting.

@MarcoMeijer
Copy link
Contributor

This issue also applies to non 9-sliced textures

In my game I have this issue with regular ui images that are close to each other:
Screenshot 2024-08-17 at 16 18 46

This issue was not there in 0.13

@ickshonpe ickshonpe self-assigned this Aug 23, 2024
@brandon-reinhart
Copy link
Contributor

Ah, I saw something like this happen when my desktop scale was not 1.0.

github-merge-queue bot pushed a commit that referenced this issue Sep 2, 2024
# Objective

Fixes #14183

## Solution

Reimplement the UI texture atlas slicer using a shader. 

The problems with #14183 could be fixed more simply by hacking around
with the coordinates and scaling but that way is very fragile and might
get broken again the next time we make changes to the layout
calculations. A shader based solution is more robust, it's impossible
for gaps to appear between the image slices with these changes as we're
only drawing a single quad.

I've not tried any benchmarks yet but it should much more efficient as
well, in the worst cases even hundreds or thousands of times faster.

Maybe could have used the UiMaterialPipeline. I wrote the shader first
and used fat vertices and then realised it wouldn't work that way with a
UiMaterial. If it's rewritten it so it puts all the slice geometry in
uniform buffer, then it might work? Adding the uniform buffer would
probably make the shader more complicated though, so don't know if it's
even worth it. Instancing is another alternative.

## Testing
The examples are working and it seems to match the old API correctly but
I've not used the texture atlas slicing API for anything before, I
reviewed the PR but that was back in January.

Needs a review by someone who knows the rendering pipeline and wgsl
really well because I don't really have any idea what I'm doing.
@Litttlefish
Copy link
Contributor Author

Litttlefish commented Sep 3, 2024

This issue also applies to non 9-sliced textures

In my game I have this issue with regular ui images that are close to each other: Screenshot 2024-08-17 at 16 18 46

This issue was not there in 0.13

Now this issue is closed, you can try on 0.14.2🤔
if it stays still you can open a new issue I think
@MarcoMeijer

@alice-i-cecile
Copy link
Member

Well, once we release 0.14.2 😂 Not out yet!

mockersf pushed a commit that referenced this issue Sep 5, 2024
Fixes #14183

Reimplement the UI texture atlas slicer using a shader.

The problems with #14183 could be fixed more simply by hacking around
with the coordinates and scaling but that way is very fragile and might
get broken again the next time we make changes to the layout
calculations. A shader based solution is more robust, it's impossible
for gaps to appear between the image slices with these changes as we're
only drawing a single quad.

I've not tried any benchmarks yet but it should much more efficient as
well, in the worst cases even hundreds or thousands of times faster.

Maybe could have used the UiMaterialPipeline. I wrote the shader first
and used fat vertices and then realised it wouldn't work that way with a
UiMaterial. If it's rewritten it so it puts all the slice geometry in
uniform buffer, then it might work? Adding the uniform buffer would
probably make the shader more complicated though, so don't know if it's
even worth it. Instancing is another alternative.

The examples are working and it seems to match the old API correctly but
I've not used the texture atlas slicing API for anything before, I
reviewed the PR but that was back in January.

Needs a review by someone who knows the rendering pipeline and wgsl
really well because I don't really have any idea what I'm doing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-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.