-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add wesnoth.interface.add_floating_label as a replacement for wesnoth.print #5837
Conversation
f952efa
to
619b40e
Compare
619b40e
to
cebb99b
Compare
Is there a demo showing this in action? |
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? |
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:
|
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 |
sounds just it would just end up confusing for UMC authors too. |
That's not what it's doing. It presents a simple API ( 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. |
cebb99b
to
032bd69
Compare
This fails schema validation now that #5800 has merged.
Passing a table as the first argument:
Passing a table as the second argument also has a confusing issue:
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. |
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 |
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.
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. |
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 Something that I'd really like to see is a guide for UMC authors about which of |
Well, the tutorial covers that particular functionality, and it appears to work there. |
Please respond to my "... What are the use-cases where the new [print] functionality is the best choice?" question.
Murphy's Law (build is master + f0cf798)
|
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. |
It's from the tutorial. |
f0cf798
to
616f465
Compare
Well, I'm not getting that error at all. I would expect to see that error if you tried to do something like |
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.
|
That's the only new functionality added to |
Nothing requires
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 |
False. In current master,
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…
This was more my intention. Yeah, you do have a point, maybe I should scale the offset by the resolution? |
Okay… so does the changed API and the added |
… 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.
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? |
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. |
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:
|
[lua] | ||
code=<< | ||
left_label_id = {valid = false} | ||
right_label_id = left_label_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While copying the values like this works, it would be more readable to just put {valid = false}
on each line.
left_label_id = {valid = false} | ||
right_label_id = left_label_id | ||
top_label_id = left_label_id | ||
bottom_label_id = left_label_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no WML API yet but I assume there would be at some point.
} 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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alpha channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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…
} | ||
if(luaW_tableget(L, 2, "color")) { | ||
if(lua_isstring(L, -1)) { | ||
color = color_t::from_hex_string(lua_tostring(L, -1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer having only one way to provide arguments, so only accepting an RGB (or RGBA) vector for the color value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting between the two formats is non-trivial, so I think it's totally worth it to support both.
Text at the bottom of the screen is going to overlap with 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 Code is in the https://github.com/stevecotton/wesnoth/commits/tutorial_overlay_text branch |
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. |
* - 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be renamed intf_add_overlay_text.
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.