devilutionX icon indicating copy to clipboard operation
devilutionX copied to clipboard

Make function to display multiple lines in panels; implement it as POC.

Open burningserenity opened this issue 2 years ago • 16 comments

Adds a function to display multiple lines of panel text, instead of having to call AddPanelString multiple tiimes.

burningserenity avatar Aug 21 '22 20:08 burningserenity

Since the text rendere can render multiple lines, maybe we can feed it just a single text, and just adjust the y offset of the first line and line height depending on the number of lines?

AJenbo avatar Aug 22 '22 00:08 AJenbo

Since the text rendere can render multiple lines, maybe we can feed it just a single text, and just adjust the y offset of the first line and line height depending on the number of lines?

Is that under engine/, or somewhere else?

burningserenity avatar Aug 22 '22 00:08 burningserenity

https://github.com/diasurgical/devilutionX/blob/master/Source/engine/render/text_render.hpp#L187

AJenbo avatar Aug 22 '22 00:08 AJenbo

I've added the SplitByChar function in #5306 under utils/str_split.hpp

glebm avatar Aug 29 '22 13:08 glebm

https://github.com/diasurgical/devilutionX/blob/master/Source/engine/render/text_render.hpp#L187

I played with breaking up text by newlines in the DrawString function, but the problem with this approach is that I need any panels drawn before and after a string with multiple lines to also have their y offset changed, but I don't have access to text from previous panels in DrawString. Am I missing something?

burningserenity avatar Sep 02 '22 23:09 burningserenity

Don't support a mix of multi line an single line with line breaks. Ideally we would only support a single entry with line breaks. Multiple lines should be migrated to single line with line breaks and support for multi line removed as soon as this works.

AJenbo avatar Sep 03 '22 00:09 AJenbo

So we would have a single entry comprised of all the text from various sources that would make up that info panel, feed them into DrawString, and break them all up in there?

burningserenity avatar Sep 03 '22 00:09 burningserenity

Might not even have to break them up, just count the line breaks, and adjust the line height and y offset before drawing the text.

AJenbo avatar Sep 03 '22 00:09 AJenbo

Okay so I have to figure out how to get all of the panel strings to be passed to DrawString. How does DrawString work in relation to AddPanelString?

burningserenity avatar Sep 03 '22 00:09 burningserenity

Well no, you just need get PrintInfo() to count the line breaks in InfoString, instead of using pnumlines.

At that point (possibly as a follow up PR) multiple calls to AddPanelString() can be replaced by setting/concat InfoString.

InfoString = string_view(item._iIName);
AddPanelString(_("Right-click to read, then"));
AddPanelString(_("left-click to target"));
InfoString = StrCat(item._iIName, "\n", _("Right-click to read, then\nleft-click to target"));

AJenbo avatar Sep 03 '22 00:09 AJenbo

Okay that was a pretty simple change. The more complicated part is how to calculate the y offset. How does the y offset get calculated with AddPanelString?

burningserenity avatar Sep 04 '22 18:09 burningserenity

LineStart[pnumlines], you already changed this meaning that it will be incorrect when using AddPanelString():

https://github.com/diasurgical/devilutionX/pull/5267/files#diff-d59ae24c872d55375ef9c864346e2841b9075ceca2b2d24f95836c67a31669f8R270

AJenbo avatar Sep 04 '22 19:09 AJenbo

I see that scrolls with stat requirements display differently, due to the AddPanelString.

burningserenity avatar Sep 04 '22 19:09 burningserenity

A picture would be helpful

AJenbo avatar Sep 04 '22 19:09 AJenbo

Screenshot_2022-09-04_15:20:11 Screenshot_2022-09-04_15:20:22 Screenshot_2022-09-04_15:20:31 Screenshot_2022-09-04_15:20:37 Screenshot_2022-09-04_15:20:39

That's with the current value, LineStart[infoStringLines + pnumlines]

burningserenity avatar Sep 04 '22 19:09 burningserenity

Think I finally grokked it:

Screenshot_2022-09-04_16:32:53 Screenshot_2022-09-04_16:32:55 Screenshot_2022-09-04_16:32:57 Screenshot_2022-09-04_16:32:58 Screenshot_2022-09-04_16:32:59 Screenshot_2022-09-04_16:33:05 Screenshot_2022-09-04_16:33:06

burningserenity avatar Sep 04 '22 20:09 burningserenity

@burningserenity FYI, you might want to have a look at #5367 and #5369. My reviews on this PR helped shape the work I did there so I hope you don't feel I'm undercutting your effort here. I also don't want you to be spinning your wheels on this for too long, and I'm hoping these two PRs will get us moving toward merging #5357.

StephenCWills avatar Sep 25 '22 02:09 StephenCWills

@burningserenity FYI, you might want to have a look at #5367 and #5369. My reviews on this PR helped shape the work I did there so I hope you don't feel I'm undercutting your effort here. I also don't want you to be spinning your wheels on this for too long, and I'm hoping these two PRs will get us moving toward merging #5357.

Ah bummer. Oh well, I'm glad I could inspire you, at least. I'll close this and watch your two PRs for when to move on #5357 .

burningserenity avatar Sep 25 '22 14:09 burningserenity