Skip to content
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

improve tooltip placement on rh panel #8908

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

Conversation

white-haired-uncle
Copy link
Contributor

In some cases (LotI), some tooltips for items on the right hand panel of the main scenario screen do not fit. The current algo to place tips just checks to see if the tip will fit above the origin, and if not places it below (which is often a worse choice). I made a suggestion for improving this at https://forums.wesnoth.org/viewtopic.php?t=57103 where it was recommended that I submit a PR, even though I'm not a programmer, I don't know C++ (or SDL, or...), etc. So here it is, warts and all.

The first step, changes to init_label(), is a complete mess. It's clear I have no idea what I'm doing, except I know I'm doing it wrong. But it does at least allow me to demonstrate the idea. Basically, the text_width of the tooltip texture is set to 400. If a tooltip is too "tall" to fit (and assuming we can't/won't fix that), one possible choice is to stretch the width until the height fits. It turns out that this is not so easy to do with text, you can't just assume that the same data will fit in the same area by scaling the width by the same ratio as the overflow in height. Also, I have no idea how to get the height of the text so I did some really ugly stuff. It works, but it ain't right and I don't have the skills at this point to do it right.

The second piece, update_label_pos() is pretty straight forward. Just check that the tip will fit before placing it below the origin, and if not find a better way to place it. That works. Obviously I need to remove the debugging lines, but I left them in for now in case they are of use to anyone.

P.S. I did read CONTRIBUTING.md, but understood about none of it.

@babaissarkar babaissarkar added Enhancement Issues that are requests for new features or changes to existing ones. UI User interface issues, including both back-end and front-end issues. labels May 23, 2024
@babaissarkar
Copy link
Contributor

babaissarkar commented May 23, 2024

You know how to fix the whitespace CI check, right?
(run ./utils/CI/fix_whitespace.sh from the wesnoth source directory, then commit and push any changed files)

src/tooltips.cpp Outdated Show resolved Hide resolved
src/tooltips.cpp Outdated Show resolved Hide resolved
src/tooltips.cpp Outdated Show resolved Hide resolved
src/tooltips.cpp Outdated Show resolved Hide resolved
@white-haired-uncle
Copy link
Contributor Author

If I continue down the road of making the label twice (if necessary -- I moved the second time inside the if block so it wouldn't run if not needed), I have to deal with the fact that create_texture() will check to see if it has already been created, as in floating_label.cpp:

bool floating_label::create_texture()
{
        if(video::headless()) {
                return false;
        }

        if(tex_ != nullptr) {
                // Already have a texture
              return true;
        }

For testing, I just commented out return true, but that's probably not a good idea since that check is surely in there for a reason. I tried to create clear_texture(), which would set tex_ to NULL (or whatever), but I was unable to find a way to clear tex_. I suppose another route might be an optional argument to create_texture() to force re-creation. I'm really not sure how to handle this, I really hadn't expected this whole approach to survive.

@white-haired-uncle
Copy link
Contributor Author

I have applied the recommendations provided here, hopefully.

Pretty sure I figured out how to clear tex_, it works at least.

Used an iterative approach to resize the tooltip when it is too large. In a rather extreme case, it took three tries to get the texture sized right, but it did get it right (vs just using a "big" value and hoping for the best). This should be pretty rare anyway, and I suspect the performance impact is negligible, at least compared to some other things I've seen happening with extreme rapidity.

Once the texture is properly sized, I re-create it once more so that the clip size is set to the screen size. I really just do that because that's the way it was when I found it. While I really don't know what I'm talking about, it seems to me that this is probably irrelevant except in the rare case when tip.x=tip.y=0 so I doubt it's worth doing, but like I said, leaving it the way I found it.

While I still think my sizing approach will give someone a good laugh someday, until that time it does work. I can now see the information that used to be truncated by size/position.

@white-haired-uncle white-haired-uncle marked this pull request as ready for review May 23, 2024 20:45
@white-haired-uncle
Copy link
Contributor Author

white-haired-uncle commented May 24, 2024

Hmm. Plays fine for hours, then I set my resolution to 1024x768 (something I never do) so I can troubleshoot something completely unrelated, and my changes hang wesnoth in what looks like an endless loop.

Looks like I failed to account for the case where the data is just too big for the screen no matter how we shape the rectangle -- it just keeps trying the same thing over and over.

Before I push a new version, the failing windows test:

D:\a\wesnoth\wesnoth\src\tooltips.cpp(36,39): warning C4305: 'initializing': truncation from 'double' to 'float'

I'm guessing it doesn't like

static const float height_fudge = 0.95;

and wants me to declare it as a double???

@soliton-
Copy link
Member

Hmm. Plays fine for hours, then I set my resolution to 1024x768 (something I never do) so I can troubleshoot something completely unrelated, and my changes hang wesnoth in what looks like an endless loop.

Looks like I failed to account for the case where the data is just too big for the screen no matter how we shape the rectangle -- it just keeps trying the same thing over and over.

Yeah, you need to abort at some point. Maybe after 3 tries or so.

Before I push a new version, the failing windows test:

D:\a\wesnoth\wesnoth\src\tooltips.cpp(36,39): warning C4305: 'initializing': truncation from 'double' to 'float'

I'm guessing it doesn't like

static const float height_fudge = 0.95;

and wants me to declare it as a double???

It means 0.95 is a double literal. You can either change the type to double or use 0.95f to get a float literal.

src/floating_label.cpp Outdated Show resolved Hide resolved
@white-haired-uncle
Copy link
Contributor Author

  1. Use texture::reset() instead of rolling my own.
  2. Quit trying to resize label once limit is reached.
  3. Fix logic error when positioning very wide tips.
  4. Declare double literal as double, not float.

@Pentarctagon Pentarctagon requested a review from Vultraz May 30, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Issues that are requests for new features or changes to existing ones. UI User interface issues, including both back-end and front-end issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants