Skip to content

[MANUAL SQUASH] Formspec: Introduce default element styling/theming #16161

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

SmallJoker
Copy link
Member

@SmallJoker SmallJoker commented May 20, 2025

This PR is based on #16146 to ensure the best experience. This is optional.

Commit 1: (using commit f14b495 for demonstration)

  • No styling (master):
    grafik
  • With styling, before (master):
    grafik
  • With styling, after (PR):
    grafik

Commit 2++:

  • This a feature to specify the formspec appearance by texture packs. See testing instructions below.

To do

This PR is Ready for Review.

How to test

  1. Put the textures from commit f14b495 into your preferred texture pack
  2. Edit or create texture_pack.conf
  3. Add
formspec_version_theme = 9

formspec_theme = """
style_type[button,image_button;bgimg_middle=2,2,-2,-2;bgimg=button_normal.png]
style_type[button:hovered,image_button:hovered;bgimg_middle=2,2,-2,-2;bgimg=button_hovered.png]
style_type[button:pressed,image_button:pressed;bgimg_middle=2,2,-2,-2;bgimg=button_pressed.png]
bgcolor[#007733;both;#FF00FF]
"""
  1. Start Luanti
  2. The main menu should have a new appearance and the in-game inventory should look like this. unified_inventory for example:
    grafik

@SmallJoker SmallJoker added Feature ✨ PRs that add or enhance a feature Formspec labels May 20, 2025
@SmallJoker SmallJoker changed the title Pr 16161 texture pack formspec theme [NO SQUASH] Formspec: Introduce default element styling/theming May 20, 2025
@SmallJoker SmallJoker added the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label May 20, 2025
@SmallJoker
Copy link
Member Author

There is one concern: how should compatibility be handled here? Unknown properties currently result in warnings. Shall the warnings simply be skipped in this case?

@SmallJoker SmallJoker changed the title [NO SQUASH] Formspec: Introduce default element styling/theming [MANUAL SQUASH] Formspec: Introduce default element styling/theming May 24, 2025
@rubenwardy
Copy link
Contributor

rubenwardy commented May 24, 2025

It's probably a good idea for this to exactly match the style/style_type syntax - including tag names and [] - for consistency and to allow more than just style_type - what about bgcolor and backgrounds?

@SmallJoker
Copy link
Member Author

There's a little problem with the current code (as of master): We have the following which can be used separately:

  • border (C++ DrawBorder) that makes the default button appearance
  • bgimg (+ bgimg_middle) to add an image under the text and above the default button appearance (border)
  • fgimg (+ fgimg_middle) to use another image instead of text (for image_button[...])

Now the motivation of this PR is to replace the default button appearance (border) by a 9-sliced bgimg (if present). Hence, such backgrounds are no longer rendered if border=false is specified, which is technically a breaking change. Is this okay or do I need to implement more hacks to make it compatible?


[...] including tag names and [] - for consistency and to allow more than just style_type [...]

There is no convention for button naming, and those found in builtin are undocumented. Thus, style[name...] has little to no purpose here.

what about bgcolor and backgrounds?

Would you please be so nice to elaborate this a bit more? I do not understand this question.

Now with this PR I am using bgimg_middle

@SmallJoker SmallJoker force-pushed the pr_16161_texture_pack_formspec_theme branch from 2b935ed to ac1b4b4 Compare May 25, 2025 11:32
@SmallJoker SmallJoker removed the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label May 25, 2025
@rubenwardy
Copy link
Contributor

There is no convention for button naming, and those found in builtin are undocumented. Thus, style[name...] has little to no purpose here.

It's more about having a consistent format and not inventing a new one. You can just say it's a list of style_type[] tags, documentation here.

Regarding style[]: there are some conventions - cancel, for example - and a future improvement could allow selecting based on formname

what about bgcolor and backgrounds?

Would you please be so nice to elaborate this a bit more? I do not understand this question.

The bgcolor formspect element allows you to set the background color of a formspec. background/background9 lets you set the background of a formspec to an image

@SmallJoker
Copy link
Member Author

SmallJoker commented May 25, 2025

It's more about having a consistent format and not inventing a new one.

Fair. I adapted the code to have a separate list of formspec elements allowed for theming. Documented accordingly.

In devtest, the provided example code results in the following beautiful theme: (ignore the font; it's the server-sent font media test I did not yet disable)

grafik

@SmallJoker SmallJoker requested a review from rubenwardy May 29, 2025 08:47
@grorp
Copy link
Member

grorp commented May 29, 2025

Now the motivation of this PR is to replace the default button appearance (border) by a 9-sliced bgimg (if present). Hence, such backgrounds are no longer rendered if border=false is specified, which is technically a breaking change. Is this okay or do I need to implement more hacks to make it compatible?

So this means for image buttons:

  • 9-slice bgimg's are now only rendered if border = true. If a 9-slice bgimg is present, the border is hidden (effectively acting like border = false)
  • non-9-slice bgimg's still work like before, independently of border

This is inconsistent. Also, it reduces flexibility, since you can no longer have what previously was a 9-slice bgimg and border = true (although this is admittedly not a very likely case)

This also breaks compatibility in practice. Cases I found:

Little Lady: missing button background for the "Quit Little Lady" button
screenshot

Repixture: missing button backgrounds in the pause menu
screenshot

Perhaps you could instead implement a new border_img to satisfy your use case of globally replacing the border = true built-in border? I don't think re-using bgimg can possibly work in all cases, if we also consider that a button can have both border = true and a bgimg. "Implement more hacks to make it compatible" doesn't sound like a sustainable approach to me.

@SmallJoker SmallJoker added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 30, 2025
This introduces a new setting to customize the default appearance
of formspecs. Server-sent 'formspec prepends' will internally
overwrite this setting.
@SmallJoker
Copy link
Member Author

This is inconsistent. Also, it reduces flexibility, since you can no longer have what previously was a 9-slice bgimg and border = true (although this is admittedly not a very likely case)

Indeed, but I haven't yet seen any case where border = true and bgimg_middle were used at the same time. Games actively disabling the border means that it's an undesired default. Still, changing that breaks compatibility (I've seen it in Mineclonia too), thus I will have to revert that for the better or worse.

Perhaps you could instead implement a new border_img to satisfy your use case of globally replacing the border = true built-in border?

Yes, that would be an option. Then we'd have the following layers:

  • border (standard defined by IGUISkin)
  • (new) border_img (overwrites the default border style)
  • bgimg (to overlay the border, if present)
  • fgimg (image_button[...] or text (button[...])

@SmallJoker
Copy link
Member Author

And yet - it seems that bgimg_middle is supposed to replace the border. For which other reason would padding depend on this value specifically?

luanti/doc/lua_api.md

Lines 3655 to 3656 in 535d757

* padding - rect, adds space between the edges of the button and the content. This value is
relative to bgimg_middle.

@rubenwardy What was the original intend of the 9-sliced button rendering?

@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author label May 30, 2025
@rubenwardy
Copy link
Contributor

rubenwardy commented Jun 4, 2025

I don't really understand what question you're asking

bgimg_middle allows you to make scalable background images for button theming, see the wikipedia page: https://en.wikipedia.org/wiki/9-slice_scaling

The padding is used to offset the content, it's helpful for getting a pressed effect I believe

@SmallJoker
Copy link
Member Author

@rubenwardy Thanks for the explanation. I am confused becausebgimg_middle has border-like (i.e. draw3DButtonPane*) properties:

  1. It generally overlays the default border. Exception: transparent pixels in the texture
  2. padding is linked to bgimg_middle instead of being independent.
    And yet - bgimg_middle and the default border may exist simultaneously, where I cannot think of any reasonable use-case.

If the style_type[] approach should be continued, then one of the following measures is necessary:

  1. Either: add yet another set of properties as outlined above (border_img)
  2. Or: have an additional property, e.g. bgimg_is_border=true that is reset when specifying a new bgimg_middle

However, I would like to style the scrollbar and ticker buttons too. These do not always have a separate formspec element (e.g. textarea), thus CGUISkin::drawColored3DButton* need to be extended. This does not fit into the style_type[] approach. A workaround (or rather hack?) could be to create a pseudo element type as follows: style_type[_skin_button_pane:pressed;img_middle=xxxx;img=xxxx].

At this point I do not know how to proceed, as the established code does not want to fit into the feature what I would like to implement.

@rubenwardy
Copy link
Contributor

rubenwardy commented Jun 5, 2025

Thanks for the explanation. I am confused becausebgimg_middle has border-like (i.e. draw3DButtonPane*) properties:

  1. It generally overlays the default border. Exception: transparent pixels in the texture

Yeah the bgimg is supported with both border=1 and border=0. With bgimg_middle, it's most useful to replace the border completely with border=0

  1. padding is linked to bgimg_middle instead of being independent.
    And yet - bgimg_middle and the default border may exist simultaneously, where I cannot think of any reasonable use-case.

I think padding defaulting to bgimg_miggle might be done as a default to get the text insets to just work with custom bgimg_middles

If the style_type[] approach should be continued, then one of the following measures is necessary:

  1. Either: add yet another set of properties as outlined above (border_img)

  2. Or: have an additional property, e.g. bgimg_is_border=true that is reset when specifying a new bgimg_middle

Okay - so the problem is that you want to replace the default border with a bgimg 9 slice, but don't want to break mods that use border=false to hide the border and just have the button text

It feels like you're introducing a lot of complexity just because you don't like the new flat buttons.

More generally, the user request for this is #10192 which would override button themes rather than changing the default. There needs to be more thought into how the styles should cascade as the styling system isn't designed for this


However, I would like to style the scrollbar and ticker buttons too. These do not always have a separate formspec element (e.g. textarea), thus CGUISkin::drawColored3DButton* need to be extended. This does not fit into the style_type[] approach. A workaround (or rather hack?) could be to create a pseudo element type as follows: style_type[_skin_button_pane:pressed;img_middle=xxxx;img=xxxx].

At this point I do not know how to proceed, as the established code does not want to fit into the feature what I would like to implement.

This is #9692

@SmallJoker
Copy link
Member Author

SmallJoker commented Jun 5, 2025

Okay - so the problem is that you want to replace the default border with a bgimg 9 slice, but don't want to break mods that use border=false to hide the border and just have the button text

Exactly. That's currently the pain point. I will introduce skin overrides, which will allow mods (or games) to provide a uniform appearance across all elements. Thank you for linking to #9692. It's exactly what I'd also like to change - just not on a per-element basis.

It feels like you're introducing a lot of complexity just because you don't like the new flat buttons.

By overriding the draw functions in CGUISkin, this should not result in too many LoC. Let's see how simple it gets. Instead of requesting a revert of 9b2ee1d, I'm trying to find a general solution where games can benefit too.

EDIT:

More generally, the user request for this is #10192 which would override button themes rather than changing the default.

Doesn't this predate the formspec styling feature? I believe that my PR will do exactly that. The priority should be as follows:

  1. Default appearance given by CGUISkin
  2. Optional overrides of the skin <--- this PR (soon)
  3. Formspec prepends (style_type[ and style[)
  4. Formspec code (same, overrides the prepends)

@SmallJoker SmallJoker force-pushed the pr_16161_texture_pack_formspec_theme branch from c0ea395 to bd0d9a7 Compare June 5, 2025 18:35
@SmallJoker
Copy link
Member Author

SmallJoker commented Jun 5, 2025

This is what can be reached by changing the skin directly (most recent commit). By using three lines, all button-like elements can be changed to use any custom 9-slice image.
grafik

However, coloring (includes highlights) are based on the skin-provided colors, which makes the implementation more annoying than it should be...

@SmallJoker SmallJoker removed the Rebase needed The PR needs to be rebased by its author label Jun 5, 2025
@Zughy Zughy added the Rebase needed The PR needs to be rebased by its author label Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Feature ✨ PRs that add or enhance a feature Formspec Rebase needed The PR needs to be rebased by its author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants