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

GLES2 & GLES3 Fixes ninepatch margins for high resolution textures. #32170

Merged
merged 2 commits into from
Oct 9, 2019

Conversation

puthre
Copy link
Contributor

@puthre puthre commented Sep 16, 2019

GLES2 Fixes for ninepatch margins when canvas patch size is smaller than the patch texture resolution.

Scaled ninepatch margins in canvas space to be relative of the ninepatch size and not bound to the texture resolution when the patch size is smaller than the patch texture.
In this moment if you create a ninepatch with a large texture and try to render it in a smaller canvas it will be distorted (you will only see huge edges) because the texture patch margins that you set in the ninepatchrect configuration are also used in absolute value in canvas coordinates to create the patches. This pull requests fixes this by scaling these margins to be relative to the ninepatchrect size when the patch is small relative to the patch texture.

This is particularly important when you need to have higher resolution ninepatch textures for higher resolution devices but have a relatively small canvas size.

Note: This only fixes GLES2, GLES3 seems to be processed in a shader but doesn't work for me, it appears distored even with lower resolution textures.

@puthre puthre requested a review from reduz as a code owner September 16, 2019 22:13
@puthre puthre changed the title GLES2 Fixes ninepatch margins for high (or low) resolution textures. GLES2 Fixes ninepatch margins for high resolution textures. Sep 17, 2019
… patch texture resolution..

Scaled ninepatch margins in screen space to be relative of the ninepatch size when the patch size is smaller than the patch texture resolution.
@akien-mga akien-mga added this to the 3.2 milestone Sep 20, 2019
@clayjohn
Copy link
Member

@puthre Can you upload the texture you used to test this? I don't have any assets I can use to test this PR, but I would like to test before approving. Thanks in advance. :)

@puthre
Copy link
Contributor Author

puthre commented Sep 20, 2019

@clayjohn Sure, here you are.
popup1

@clayjohn
Copy link
Member

I can confirm this fixes the issue in GLES2. We need to make the behaviour match in GLES3 before merging though.

@clayjohn
Copy link
Member

In GLES3 within canvas.glsl, color_texpixel_size is equivalent to 1.0 / np->source.size and dst_rect is equivalent to np->rect. So using those you can do a straight port of your work here into GLES3. :)

@puthre
Copy link
Contributor Author

puthre commented Sep 26, 2019

In GLES3 within canvas.glsl, color_texpixel_size is equivalent to 1.0 / np->source.size and dst_rect is equivalent to np->rect. So using those you can do a straight port of your work here into GLES3. :)

The main problem for me was that the gles3 version seemed to have some other problems as it was behaving differently and not in a good way from gles2 (before this patch). I'll try to find the time to look at it in more depth.

@clayjohn
Copy link
Member

That would be great! Let me know if you need any help. :)

… patch texture resolution

Scaled ninepatch margins in screen space to be relative of the ninepatch size when the patch size is smaller than the patch texture resolution.
@puthre
Copy link
Contributor Author

puthre commented Oct 7, 2019

@clayjohn I think I got it

@akien-mga akien-mga changed the title GLES2 Fixes ninepatch margins for high resolution textures. GLES2 & GLES3 Fixes ninepatch margins for high resolution textures. Oct 8, 2019
@akien-mga akien-mga requested a review from clayjohn October 8, 2019 15:06
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@akien-mga akien-mga merged commit a39aead into godotengine:master Oct 9, 2019
@akien-mga
Copy link
Member

Thanks!

@Shadowblitz16
Copy link

Shadowblitz16 commented Jan 20, 2020

this needs to either be reverted or have a flag to enable it because it compleatly breaks compatibility and isn't how 9 slices should work.

what should happen is if you shrink your 9 slice smaller then the texture it should start clipping the middle and if its less then the middle it should start clipping the edges NOT mess with them

@qq715152910
Copy link

this needs to either be reverted or have a flag to enable it because it compleatly breaks compatibility and isn't how 9 slices should work.

what should happen is if you shrink your 9 slice smaller then the texture it should start clipping the middle and if its less then the middle it should start clipping the edges NOT mess with them

I had the same problem, but they ignored my existence ...
#37027

@Shadowblitz16
Copy link

this is going to make graphical gui's a pain in the ass to build.
it just proves people don't have common sense when picking what issues should and shouldn't be added.

@clayjohn
Copy link
Member

@puthre Can you weigh in here?

@Shadowblitz16 Please don't resort to personal attacks. When reviewing PR's we rely on contributors to identify best practices in areas that we are less familiar. Sometimes fixing one workflow breaks another. This can happen when the contributor only turns their mind to fixing a particular (broken) workflow and forgets about another (equally valid) worlkflow. Saying "people don't have common sense when picking what issues should and shouldn't be added" is plain rude and does not help anyone.

Since you clearly know better than everyone else, please suggest how we can fix the problem you are experiencing rather than attacking the people involved.

@Shadowblitz16
Copy link

Shadowblitz16 commented Mar 15, 2020

@clayjohn
I'm sorry if you see it that way. I am not attacking any particular person, I just don't agree with some of the decisions that are being made.
also I did

what should happen is if you shrink your 9 slice smaller then the texture it should start clipping the middle and if its less then the middle it should start clipping the edges NOT mess with them

apologies if I upset someone

@qq715152910
Copy link

qq715152910 commented Mar 15, 2020

@puthre Can you weigh in here?

@Shadowblitz16 Please don't resort to personal attacks. When reviewing PR's we rely on contributors to identify best practices in areas that we are less familiar. Sometimes fixing one workflow breaks another. This can happen when the contributor only turns their mind to fixing a particular (broken) workflow and forgets about another (equally valid) worlkflow. Saying "people don't have common sense when picking what issues should and shouldn't be added" is plain rude and does not help anyone.

Since you clearly know better than everyone else, please suggest how we can fix the problem you are experiencing rather than attacking the people involved.

I wrote a demo of the NinePatchTexture plug-in. You can compare the zoom effect and the picture quality. Maybe you will laugh out after reading ...
NinePatchTexture-Demo
Or download from here:
#37077

@puthre
Copy link
Contributor Author

puthre commented Mar 16, 2020

@clayjohn I think neither the previous behavior nor the current one are ideal:
On the previous one you could not use high resolution texture patches on low resolution canvases (but high resolution displays), the current one tries to accomodate that by stretching the edges if the texture is larger than the patch canvas size so the large margin can fit in the small screen space and at least there is no gap between the patches, but by doing so breaks the assumption that the margins are fixed and only the center shrinks (this seems to be important for pixel perfect games but can be fixed by just using a smaller texture size).
Ideally a ninepatch implementation should not depend on the underlying canvas size and should look the same on any, regardless of the resolution. It should allow to edit/specify the margins both in texture space and o screen.
If there are more people complaining about the new behaviour than the previous one and that the general agreement is that it was better the way it was before, of course I have no issue for you to just revert it. (making it optional is also an option but then we would have 2 different behaviors on two different backends - GLES2 and GLES3 to mentain).

@qq715152910
Copy link

qq715152910 commented Mar 16, 2020

@clayjohn I think neither the previous behavior nor the current one are ideal:
On the previous one you could not use high resolution texture patches on low resolution canvases (but high resolution displays), the current one tries to accomodate that by stretching the edges if the texture is larger than the patch canvas size so the large margin can fit in the small screen space and at least there is no gap between the patches, but by doing so breaks the assumption that the margins are fixed and only the center shrinks (this seems to be important for pixel perfect games but can be fixed by just using a smaller texture size).
Ideally a ninepatch implementation should not depend on the underlying canvas size and should look the same on any, regardless of the resolution. It should allow to edit/specify the margins both in texture space and o screen.
If there are more people complaining about the new behaviour than the previous one and that the general agreement is that it was better the way it was before, of course I have no issue for you to just revert it. (making it optional is also an option but then we would have 2 different behaviors on two different backends - GLES2 and GLES3 to mentain).

In Jiugong's standard algorithm:
1.The four corners should be kept (tiled) to their original size
2. The four sides should keep one-way scaling
3. The middle area should maintain two-way scaling

But I think NinePatchRect's current algorithm has lost its practical significance.
test2
Please download the new Demo again and observe its borders and corners.
NinePatchTexture-Demo

Come Here Please.
->
#37077

@qq715152910
Copy link

@clayjohn I think neither the previous behavior nor the current one are ideal:
On the previous one you could not use high resolution texture patches on low resolution canvases (but high resolution displays), the current one tries to accomodate that by stretching the edges if the texture is larger than the patch canvas size so the large margin can fit in the small screen space and at least there is no gap between the patches, but by doing so breaks the assumption that the margins are fixed and only the center shrinks (this seems to be important for pixel perfect games but can be fixed by just using a smaller texture size).
Ideally a ninepatch implementation should not depend on the underlying canvas size and should look the same on any, regardless of the resolution. It should allow to edit/specify the margins both in texture space and o screen.
If there are more people complaining about the new behaviour than the previous one and that the general agreement is that it was better the way it was before, of course I have no issue for you to just revert it. (making it optional is also an option but then we would have 2 different behaviors on two different backends - GLES2 and GLES3 to mentain).

Yes, I am in favor of this solution, adding an option to the properties panel to restore the previous logic.
After all, the wheels I made still have efficiency issues, so I still want to restore the previous features.

@Shadowblitz16
Copy link

I ran into this problem again today.
due to this issue I am calling the current gui system unusable if you want to do pixelart

why would someone want to shrink pixels in the first place? a pixel is a pixel if you want to scale it down make smaller pixel art or scale down the window surface.

I think this change should be completely removed the logic doesn't even make sense on why this was implemented in the first place

if this has to do with textures scale not being related to window canvas scale then that should be fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants