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

Change the NinePatch behaviour #1618

Closed
lawnjelly opened this issue Oct 6, 2020 · 4 comments
Closed

Change the NinePatch behaviour #1618

lawnjelly opened this issue Oct 6, 2020 · 4 comments
Milestone

Comments

@lawnjelly
Copy link
Member

lawnjelly commented Oct 6, 2020

Describe the project you are working on:
I'm just implementing unified batching, this should end up being the new main 2d renderer for GLES2 and GLES3.

Describe the problem or limitation you are having in your project:
I'm currently implementing ninepatches, they are drawn by translating the ninepatch into 9 rects on the CPU. This is far simpler than the current method of trying to handle them as a totally different primitive. I have initially duplicated the simple behaviour of GLES2, supporting only the stretch mode.

There seems to be some controversy in how nine patches should work between before and after puthre's PR:

godotengine/godot#32170
godotengine/godot#33225

I am in agreement with some of the posters that if we are introducing a new mode (puthre's) we should probably make it switchable between the old and new. A lot of posters seem to prefer the previous method and as a layman, that would be what I would expect ninepatch to do (keep sizes of borders pixel constant and resize the central patch). So it may be worth changing the default back to the old method and having the new method as an option.

If we went with the new method being an option we could either have this settable on each ninepatch, or have a global setting for ninepatch behaviour. Let me know any thoughts on this.

I've also just been looking at the tiling modes (which are only supported in GLES3, via the shader) trying to work out how they are intended to work. It turns out that because the tiling is limited to the central patch, this can't be done with hardware tiling and has to rely on manual tiling in the fragment shader. This will cause very low performance on some GLES2 devices and graphical anomalies. So for now I'll probably support batching for stretch mode only, and revert to the legacy method for tiled modes in GLES3.

Also topic is a good opportunity for any other ideas about ninepatches to put them forward.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
Selection between the non-scaling standard ninepatch method and puthre's method. Either as a project setting or per ninepatch.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
Should the top left corner be shrinking in the right hand version? Or should the pixel sizes be preserved?
ninepatch_shrink

Ninepatch project setting:
ninepatch_mode

If this enhancement will not be used often, can it be worked around with a few lines of script?:
N/A

Is there a reason why this should be core and not an add-on in the asset library?:
N/A

@KoBeWi
Copy link
Member

KoBeWi commented Oct 6, 2020

See also godotengine/godot#37077

I've ran into this only once, but I can agree that the old behavior is more expected.

@Calinou Calinou changed the title Ninepatch behaviour Change the NinePatch behaviour Oct 6, 2020
@lawnjelly
Copy link
Member Author

This project setting switch is now implemented in unified batching, you can switch between the old method (default) and puthre's method. It is also implemented in legacy GLES2. For GLES3 legacy the shader needs modifying and I need to consult with @clayjohn on whether this is a good idea for that path.

@clayjohn
Copy link
Member

clayjohn commented Oct 6, 2020

I think the old method is probably best. There was a lot of pushback when the behaviour originally changed.

@lawnjelly
Copy link
Member Author

Implemented in unified batching.

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

No branches or pull requests

3 participants