Add wesnoth.interface.add_floating_label as a replacement for wesnoth.print #5837

CelticMinstrel · 2021-06-07T04:14:05Z

This returns a label handle which allows you to remove, reposition, or replace the label later.

In addition to all the features of wesnoth.print, you can now specify where the label appears onscreen,
as well as a fadeout time separate from the duration.

stevecotton · 2021-06-07T10:31:58Z

This returns a label handle which allows you to remove, reposition, or replace the label later.

Is there a demo showing this in action?

CelticMinstrel · 2021-06-07T13:11:07Z

There's no demo; I tested by launching the tutorial and running the commands from the console. Shouldn't be too hard to make a demo, I guess?

stevecotton · 2021-06-12T06:48:19Z

Specifically 5781, 5837, and 5852.
All except 5852 has already had some review.

Just because someone's asked for a demo doesn't mean that they've reviewed the code. I want unit tests and/or a demo before I review the code.

Although this was reviewed back in #5537, the comments raised there don't seem to have been addressed:

There will be both wesnoth.interface.float_label and wesnoth.interface.add_floating_label, which despite the similar names are different implementations.

The arguments for add_floating_label aren't documented in the C++, which is a break from the coding style in this file. The code also seems to be trying to accept options in multiple orders (and treating hexadecimal strings as a different thing to numbers), which is adding unnecessary complexity.

CelticMinstrel · 2021-06-12T16:09:10Z

I didn't address the name similarity simply because I haven't been able to come up with a better name. I can add documentation comments to add_floating_label. (There are other functions that lack this documentation too, this should probably be addressed someday.)

Pentarctagon · 2021-06-12T17:16:30Z

trying to accept options in multiple orders

sounds just it would just end up confusing for UMC authors too.

CelticMinstrel · 2021-06-12T17:37:11Z

sounds just it would just end up confusing for UMC authors too.

That's not what it's doing. It presents a simple API (add_floating_label( "text", duration, location) ) but allows the first argument to also be an options table (to control text size and color) and the same for the second argument (letting you specify the fade-out duration separately).

Regardless of the form you choose, the first argument is the text, the second argument is the duration, and the final argument is the location.

stevecotton · 2021-06-16T06:36:36Z

This fails schema validation now that #5800 has merged.

trying to accept options in multiple orders

That's not what it's doing.

Passing a table as the first argument:

{"Label", "ff8000", 42} is equivalent to {"Label", 42, "ff8000", 42}
{"Label", "42", "ff8000"} gives the error "color hex string should be exactly 6 digits"
{"Label", 42, 0x0080ff} crashes out "Caught general 'St12length_error' exception: Text is too long to render", because it's interpreting the color as a size

Passing a table as the second argument also has a confusing issue:

{duration = "infinity"} doesn't trigger a warning, but the label fades out after 2 seconds

The demo doesn't include full coverage of the features in this PR.

CelticMinstrel · 2021-06-16T13:57:23Z

The demo doesn't include full coverage of the features in this PR.

It wasn't really meant to have full coverage… it does cover most of the new features though? I guess there's no label with a separate duration and fade time, but that's the only thing I can think of.

stevecotton · 2021-06-16T21:16:01Z

Instead of having several arguments that can contain tables of optional values, why not have the first argument always be the mandatory text string, and the second argument be a table of all optional values?

About code coverage - if there's functionality that isn't needed by the demo and isn't needed by the [print] tag's implementation, is it necessary to add that functionality now?

CelticMinstrel · 2021-06-17T00:42:28Z

Instead of having several arguments that can contain tables of optional values, why not have the first argument always be the mandatory text string, and the second argument be a table of all optional values?

Well, I like how this method groups the options into related sets - the first argument describes the text, the second argument describes the duration, and the last argument describes the positioning. Having a single options table for everything would certainly be a possibility though.

About code coverage - if there's functionality that isn't needed by the demo and isn't needed by the [print] tag's implementation, is it necessary to add that functionality now?

I think it's useful functionality. Even though it's not covered by the demo, I'm pretty sure I did some tests back when I implemented it, to make sure it works. I can do some more, just to make sure.

stevecotton · 2021-06-17T11:26:42Z

Even though it's not covered by the demo, I'm pretty sure I did some tests back when I implemented it, to make sure it works. I can do some more, just to make sure.

Thanks for the honesty, but I'd have more confidence if the demo code had full coverage.

The more functionality that you add, the harder it becomes for someone to later try and unify this with the [floating_text] tag and the [label] tag, and also the more time that UMC authors are likely to waste working out which of those tags does what they want. I'd much prefer to keep [print] as simple as possible; that also makes it more likely that someone in future will agree to adding a function with the name that you want, and with the full functionality that you want.

Something that I'd really like to see is a guide for UMC authors about which of [floating_text], [label] and [print] they should use. What are the use-cases where the new [print] functionality is the best choice?

CelticMinstrel · 2021-06-17T13:48:15Z

Thanks for the honesty, but I'd have more confidence if the demo code had full coverage.

Well, the tutorial covers that particular functionality, and it appears to work there.

data/campaigns/tutorial/utils/utils.cfg

stevecotton · 2021-06-17T16:06:40Z

Please respond to my "... What are the use-cases where the new [print] functionality is the best choice?" question.

Well, the tutorial covers that particular functionality, and it appears to work there.

Murphy's Law (build is master + f0cf798)

error scripting/lua: lua/wml-tags.lua:795: bad argument #2 to 'add_overlay_text' (that duration should be integer or 'infinity' expected, got table)
stack traceback:
	[C]: in field 'add_overlay_text'
	lua/wml-tags.lua:795: in local 'cmd'
	lua/wml-utils.lua:144: in field 'handle_event_commands'
	lua/wml-flow.lua:5: in function <lua/wml-flow.lua:4>

CelticMinstrel · 2021-06-17T18:23:30Z

Is that from the tutorial, or from manually calling the function (eg in the Lua console)? Because I distinctly remember a lack of error messages when I tested the tutorial.

stevecotton · 2021-06-17T18:58:41Z

It's from the tutorial.

CelticMinstrel · 2021-06-18T01:14:36Z

Well, I'm not getting that error at all. I would expect to see that error if you tried to do something like {duration = 'moo'}.

stevecotton · 2021-06-18T03:57:33Z

Please respond to my "... What are the use-cases where the new [print] functionality is the best choice?" question.

I retested and reproduced that error on 616f465.

Start the tutorial
Choose which character to play
Click on the character
Error message appears (and the [print]'d text does not appear)

CelticMinstrel · 2021-06-18T04:22:46Z

A separate fade time just makes sense most of the time - gradually fading out over the full duration is quite weird.
Setting an offset would be useful if you want to have several types of messages appear on different parts of the screen.

That's the only new functionality added to [print]; the Lua API supports moving an existing label and placing multiple labels onscreen at the same time, but [print] doesn't.

stevecotton · 2021-06-18T05:11:02Z

A separate fade time just makes sense most of the time - gradually fading out over the full duration is quite weird.

Nothing requires [print] to have a fade-out at all, and it doesn't in the current master.

Setting an offset would be useful if you want to have several types of messages appear on different parts of the screen.

If they're meant to align to things on the map then something that takes a location would be better than something that takes an offset. If the intention is to place things somewhere on the user's screen then an API like IntroWML that adjusts to the screen size would be better than using pixel offsets.

In short, I don't see a use case which would justify making [print] more complex. It's probably time to ask @Pentarctagon to decide if this PR should go in.

CelticMinstrel · 2021-06-18T13:40:02Z

Nothing requires [print] to have a fade-out at all, and it doesn't in the current master.

False. In current master, [print]'s fade-out is its duration. Just try waiting for ten seconds in the tutorial.

If they're meant to align to things on the map then something that takes a location would be better than something that takes an offset.

Yeah, I was thinking about that too… I'd like to add support for linking the location to a map location but I'm not really sure how that would work…

If the intention is to place things somewhere on the user's screen then an API like IntroWML that adjusts to the screen size would be better than using pixel offsets.

This was more my intention. Yeah, you do have a point, maybe I should scale the offset by the resolution?

CelticMinstrel · 2022-03-20T22:32:55Z

Okay… so does the changed API and the added [hv]align keys address most of your concerns? I'll add "top" and "bottom" labels to the test scenario, but is there anything else I should add as well?

… of the screen This is probably still not "correct" but it seems closer. I don't know of any way to determine the line height of a font size, unfortunately.

…turn

CelticMinstrel · 2022-03-20T23:02:52Z

Noticed a bug – labels don't reposition correctly if you switch resolution. I feel like this should be a solved problem in the engine though… or do other built-in labels do the same thing?

src/scripting/game_lua_kernel.cpp

stevecotton · 2022-03-25T20:52:48Z

Noticed a bug – labels don't reposition correctly if you switch resolution. I feel like this should be a solved problem in the engine though… or do other built-in labels do the same thing?

I feel investigating that, or working out how to connect the implementation of this to the window-resize, is part of this PR. Should I review before that's implemented?

CelticMinstrel · 2022-03-26T01:50:29Z

I'd like a review of the API at least. If you don't want to comb through the source code when you're not sure what might change for the bugfix, that seems fine to me.

CelticMinstrel · 2022-03-27T16:55:24Z

Okay, so I tested in 1.14 and 1.16, and the bug about labels not adjusting when you switch resolutions is already present, so I think that bug shouldn't be a blocker for merging this PR and should instead be opened as a separate issue.

What I did:

Set Wesnoth to 1920x1080
Start the tutorial. See the "Click on" message onscreen
Set Wesnoth to 1024x768
Observe that the "Click on" message is now overlapping the sidebar

stevecotton · 2022-04-04T21:21:58Z

data/test/scenarios/overlay_text_demo.cfg

+        [lua]
+            code=<<
+                left_label_id = {valid = false}
+                right_label_id = left_label_id


While copying the values like this works, it would be more readable to just put {valid = false} on each line.

stevecotton · 2022-04-04T21:43:21Z

data/test/scenarios/overlay_text_demo.cfg

+                left_label_id = {valid = false}
+                right_label_id = left_label_id
+                top_label_id = left_label_id
+                bottom_label_id = left_label_id


Wondering about the WML API that would be built over this, and how it would fit with multiple labels. I think the current [print] behavior of always replacing a previous label should be applied in a per-location fashion, so [add_overlay_text]valign=bottom would replace the previous [add_overlay_text]valign=bottom etc.

Or, if there isn't a WML wrapper, will it always need a prestart event like this one?

#define INIT_HINT_STRING_SUPPORT # Should be used in [event]name=prestart, or another event that triggers before the first hint. [lua] code=<< hint_label = {valid = false} >> [/lua] #enddef

Context: I've been wondering about this as a solution to the unread walls of text in the tutorial.

#define SHOW_HINT_STRING TEXT # Similar to PRINT_STRING, but puts the text at the edge (top or bottom) of the screen. # Used in the tutorial for info that can be left for reading until another hint is triggered. # # There is no interaction between PRINT_STRING and SHOW_HINT_STRING - they don't cancel each # other's messages. [lua] code=<< hint_label = wesnoth.interface.add_overlay_text((...).text, { size = 24, color = {255, 255, 255}, duration = "unlimited", valign = "bottom" }) >> [args] text={TEXT} [/args] [/lua] #enddef

There is no WML API yet but I assume there would be at some point.

stevecotton · 2022-04-04T21:45:46Z

src/scripting/game_lua_kernel.cpp

+			} else {
+				auto vec = lua_check<std::vector<int>>(L, -1);
+				if(vec.size() != 3) {
+					return luaL_error(L, "floating label text color should be a hex string or an array of 3 integers");


Alpha channel?

That is a good question… I'm not even sure if alpha channel would work, but I also can't think of a reason why it wouldn't…

stevecotton · 2022-04-04T21:58:04Z

src/scripting/game_lua_kernel.cpp

+		}
+		if(luaW_tableget(L, 2, "color")) {
+			if(lua_isstring(L, -1)) {
+				color = color_t::from_hex_string(lua_tostring(L, -1));


I'd prefer having only one way to provide arguments, so only accepting an RGB (or RGBA) vector for the color value.

Converting between the two formats is non-trivial, so I think it's totally worth it to support both.

stevecotton · 2022-04-06T00:21:29Z

Text at the bottom of the screen is going to overlap with [message] boxes, so maybe that area isn't useful. Text at the top seems promising, if it's large font and isn't in a multiplayer scenario (so there's no chat log).

I tried using this as an improvement for the tutorial's walls-of-text, by converting some of the narrator messages to overlays. I doubt that's what you expected, but it's one of the UXs that I think this could be useful for. It would either need much less text to support a bigger font, or it would need a background similar to the one for [message].

Code is in the https://github.com/stevecotton/wesnoth/commits/tutorial_overlay_text branch

CelticMinstrel · 2022-04-06T00:50:57Z

It would either need much less text to support a bigger font, or it would need a background similar to the one for [message].

I think the floating label system does support a background, so that could be added to the Lua API either in this PR or in a later PR.

…absolute width

stevecotton · 2022-04-21T14:25:46Z

src/scripting/game_lua_kernel.cpp

+ * - halign: horizontal alignment and anchoring - "left", "center", or "right"
+ * Returns: label handle
+ */
+int game_lua_kernel::intf_set_floating_label(lua_State* L, bool spawn)


Should probably be renamed intf_add_overlay_text.

CelticMinstrel mentioned this pull request Jun 7, 2021

Another round of Lua reorganization work #5781

Merged

CelticMinstrel force-pushed the lua_floating_label branch from f952efa to 619b40e Compare June 7, 2021 04:23

github-actions bot added Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign Lua API Issues with the Lua engine and API. labels Jun 7, 2021

CelticMinstrel force-pushed the lua_floating_label branch from 619b40e to cebb99b Compare June 7, 2021 05:49

CelticMinstrel force-pushed the lua_floating_label branch from cebb99b to 032bd69 Compare June 12, 2021 19:22

github-actions bot added the Unit Tests Issues involving Wesnoth's unit test suite. label Jun 12, 2021

github-actions bot added the Schema label Jun 17, 2021

stevecotton reviewed Jun 17, 2021

View reviewed changes

data/campaigns/tutorial/utils/utils.cfg Outdated Show resolved Hide resolved

CelticMinstrel force-pushed the lua_floating_label branch from f0cf798 to 616f465 Compare June 18, 2021 01:13

CelticMinstrel added 5 commits March 20, 2022 18:46

Add tests for top and bottom labels to the test scenario

ec8f83b

Adjust the adjustment for avoiding bottom labels going off the bottom…

c424f41

… of the screen This is probably still not "correct" but it seems closer. I don't know of any way to determine the line height of a font size, unfortunately.

Explanatory comment

e98965f

Mostly cleanup?

b8ec8de

Making the test leaders quick means you can reach two castles in one …

d7d5a13

…turn

stevecotton reviewed Mar 21, 2022

View reviewed changes

src/scripting/game_lua_kernel.cpp Outdated Show resolved Hide resolved

Comment clarification

e4a41e7

stevecotton mentioned this pull request Mar 22, 2022

Wishlist: disable the whiteboard during the tutorial (GNA #19223) #1285

Open

stevecotton reviewed Apr 4, 2022

View reviewed changes

Address a feedback comment

fd69e10

This was referenced Apr 6, 2022

Proof of concept: Convert the tutorial's wall of text [message]s into overlay text #6615

Closed

Tutorial S02: Reduce the number of [messages] #6614

Closed

CelticMinstrel added 5 commits April 9, 2022 15:12

Oops! Forgot to add the luadoc!!!

af4a851

Add a max_width parameter

cb8d001

Add bgcolor and bgalpha parameters

4c658d9

Oh I just noticed the C++ doc comment was outdated...

2c90f35

max_width can also be a percentage of the screen width instead of an …

13868fb

…absolute width

CelticMinstrel merged commit a76aa9b into master Apr 10, 2022

CelticMinstrel deleted the lua_floating_label branch April 10, 2022 17:00

stevecotton reviewed Apr 21, 2022

View reviewed changes

Add wesnoth.interface.add_floating_label as a replacement for wesnoth.print #5837

Add wesnoth.interface.add_floating_label as a replacement for wesnoth.print #5837

Conversation

CelticMinstrel commented Jun 7, 2021

stevecotton commented Jun 7, 2021

CelticMinstrel commented Jun 7, 2021

stevecotton commented Jun 12, 2021

CelticMinstrel commented Jun 12, 2021

Pentarctagon commented Jun 12, 2021

CelticMinstrel commented Jun 12, 2021

stevecotton commented Jun 16, 2021

CelticMinstrel commented Jun 16, 2021

stevecotton commented Jun 16, 2021

CelticMinstrel commented Jun 17, 2021

stevecotton commented Jun 17, 2021

CelticMinstrel commented Jun 17, 2021

stevecotton commented Jun 17, 2021

CelticMinstrel commented Jun 17, 2021

stevecotton commented Jun 17, 2021

CelticMinstrel commented Jun 18, 2021

stevecotton commented Jun 18, 2021

CelticMinstrel commented Jun 18, 2021

stevecotton commented Jun 18, 2021

CelticMinstrel commented Jun 18, 2021

CelticMinstrel commented Mar 20, 2022

CelticMinstrel commented Mar 20, 2022

stevecotton commented Mar 25, 2022

CelticMinstrel commented Mar 26, 2022

CelticMinstrel commented Mar 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevecotton commented Apr 6, 2022

CelticMinstrel commented Apr 6, 2022

Choose a reason for hiding this comment

CelticMinstrel commented Mar 27, 2022 •

edited