Fix tiled BackgroundPainter performance #2

BlazingTwist · 2023-02-03T06:29:45Z

Wombo Combo Pull Request LibGui PR

I'm sorry this is such a massive change, but there was no fixing this without a sledgehammer.

The essence of the changes is to allow the GPU to handle tiling, rather than doing it on the CPU.

Why I believe you should merge this:

I'll let the screenshot speak for itself.

Before:

After:

Juuxel

Thanks for the PR! Some notes, and see my individual review comments as well.

TextureRegion should be left intact along with the methods that use it.
This change can be completely done without breaking changes and without removing functionality in the form of removing
- texture regions
- computing edge texture regions automatically based on a cornerUv builder method

Juuxel · 2023-02-03T16:03:58Z

src/main/java/juuxel/libninepatch/NinePatch.java

-        /** Regions of the texture are tiled to fill areas. The default value. */
+        /**
+         * Regions of the texture are tiled to fill areas. The default value.
+         */
        TILING,
-        /** Regions of the texture are stretched to fill areas. */
+        /**
+         * Regions of the texture are stretched to fill areas.
+         */


Existing code should not be reformatted.

Juuxel · 2023-02-03T16:04:25Z

src/main/java/juuxel/libninepatch/NinePatch.java

+                                         T topLeft, T topCenter, T topRight,
+                                         T centerLeft, T centerCenter, T centerRight,
+                                         T bottomLeft, T bottomCenter, T bottomRight) {


The edge and corner textures should be optional additions in the builder.

Juuxel · 2023-02-03T16:05:03Z

src/main/java/juuxel/libninepatch/NinePatch.java

-        private float cornerU = 0f;
-        private float cornerV = 0f;


This functionality should remain as a fallback option for when texture regions for the sides and corners are not specified manually.

Juuxel · 2023-02-03T16:05:18Z

src/main/java/juuxel/libninepatch/NinePatch.java

+                        T centerLeft, T centerCenter, T centerRight,
+                        T bottomLeft, T bottomCenter, T bottomRight) {
+            //noinspection unchecked
+            this.textures = (T[][]) Array.newInstance(textureClass, 3, 3);


Separate fields for all of them would be better, no need to have an array.

BlazingTwist · 2023-02-03T16:39:28Z

While I believe that the TextureRegion is a concept that should not be forced onto TextureRenderer implementations, I understand wanting to avoid breaking changes.

I'll have a look what I can do tomorrow-ish.

Juuxel · 2023-02-03T19:20:08Z

TextureRegion is not really forced onto TextureRenderer implementations since the methods using it are default utility methods. The actual renderer only has to deal with its own T value combined with adjusted UV coordinates. (Unless they want to override the region methods too.)

This reverts commit 3afe842.

BlazingTwist · 2023-02-08T13:53:01Z

Alright, I did a re-design with preservation of TextureRegion in mind.

One crucial aspect here is eliminating all NinePatch specific UV coordinates before rendering occurs.
If TextureRenderer implementations wish to use modified UV coordinates they can just use an appropriate TextureType and corresponding textureAllocator.

(Once this PR reaches a merge-ready state in your eyes, I'll update the LibGUI PR)

Juuxel

Looks a lot better, but still:

I'm not planning to commit breaking changes. LNP is also part of LibGui's API, which would mean this change would be at earliest for MC 1.20 snapshots.
@NotNull should not be used, it's considered the default unless there's @Nullable
Type variables should not be upper camel case (TextureType) since they can be confused with actual types
I believe removing the UV coordinates will not allow drawing partial side textures like this:

BlazingTwist · 2023-02-09T01:20:39Z

I'm not planning to commit breaking changes.

As I see it, there are two remaining options:

I can add a completely separate NinePatch class for tiling support, which can be opted into with the NinePatch.Builder.
This should leave all existing usages of LNP intact, but doubles the amount of code in LNP.
LibGui would need a custom shader program that supports tiling - and I will not be the one writing that.
(edit: actually there's 3) Pretend the performance issue does not exist and use Mode.STRETCHING in LibGui.

Type variables should not be upper camel case

I believe that single letter generic types lack descriptiveness.
Hence I named it what it is, a generic Type intended for Textures.
If you prefer I can use uppercase with underscores.

I believe removing the UV coordinates will not allow drawing partial side textures like this

True, the above mentioned option 1 could resolve that.

Juuxel · 2023-02-09T15:24:39Z

The partial side textures are definitely a feature that needs to be kept, otherwise the tiles will be stretched (so there's no point in tiling to begin with) or will bleed over past the corners.

As for the generic naming, SCREAMING_SNAKE_CASE is what the JDK uses (see stream code for example), and the Type suffix is also useless. So T, TEX and TEXTURE should all be fine.

BlazingTwist · 2023-02-09T16:07:02Z

Any comment on which of the 3 options you prefer?

Juuxel · 2023-02-09T18:24:46Z

I'm actually looking into the shader approach - a bit more work on the LibGui side but really simple on LNP's and doesn't require any breaking (or otherwise major) changes. Seems to also be more performant than the current approach just like yours.

On the LNP side it's just the new drawTiled method in texture renderers from the original approach in this PR.

I'll open PRs to both projects to show my changes if you want to have a look.

Juuxel · 2023-03-17T18:07:11Z

Superseded by #3.

fix tiled BackgroundPainter performance

3afe842

BlazingTwist mentioned this pull request Feb 3, 2023

Fix tiled BackgroundPainter performance CottonMC/LibGui#183

Closed

Juuxel requested changes Feb 3, 2023

View reviewed changes

BlazingTwist added 3 commits February 8, 2023 14:46

fix tiled BackgroundPainter performance v2

ca83b1d

Revert "fix tiled BackgroundPainter performance"

e38d287

This reverts commit 3afe842.

Merge branch 'tiling-perf-fix'

85da1d4

Juuxel requested changes Feb 8, 2023

View reviewed changes

Juuxel closed this Mar 17, 2023

Fix tiled BackgroundPainter performance #2

Fix tiled BackgroundPainter performance #2

Uh oh!

Conversation

BlazingTwist commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I believe you should merge this:

Uh oh!

Juuxel left a comment

Choose a reason for hiding this comment

Uh oh!

Juuxel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

Juuxel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

Juuxel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

Juuxel Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

BlazingTwist commented Feb 3, 2023

Uh oh!

Juuxel commented Feb 3, 2023

Uh oh!

BlazingTwist commented Feb 8, 2023

Uh oh!

Juuxel left a comment

Choose a reason for hiding this comment

Uh oh!

BlazingTwist commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Juuxel commented Feb 9, 2023

Uh oh!

BlazingTwist commented Feb 9, 2023

Uh oh!

Juuxel commented Feb 9, 2023

Uh oh!

Juuxel commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BlazingTwist commented Feb 3, 2023 •

edited

Loading

BlazingTwist commented Feb 9, 2023 •

edited

Loading