raduprv / Eternal-Lands

http://www.eternal-lands.com
Other
158 stars 57 forks source link

Enabling speech bubbles is broken after the TTF change. #90

Closed pjbroad closed 3 years ago

pjbroad commented 3 years ago

ALT+O enables speech bubbles over your character for local your chat.

However, since the TTF change:

While bubble background is also not displayed correctly, this is broken before the TTF change.

In draw_ingame_string() in font.h, the chat font is used where the name font should perhaps be specified. If this is changed, the name font is still reset to font 0, but the text does now display.

The problem appears to be that the calculated font size is too small and this triggers the font reset in the FontManager::get() function. I've so far failed to find a fix. Perhaps @gvissers, you could take a look?

gvissers commented 3 years ago

Will have a look, might be a few days, however. As this is probably the most unloved feature in EL, I don't think speed is much of, though :)

gvissers commented 3 years ago

After staring at the draw_actor_overtext() and draw_ingame_string() for an hour and trying to figure out the coordinate system, I decided that trying to draw text in world coordinates is utter madness. I also think the perspective view on the text looked rather strange. Since most of the banner was drawn in orthographic coordinates already, I simply moved the speech bubbles up a bit into that code block, and scrapped draw_ingame_string() altogether. The coordinates need some fine tuning still: the speech bubbles overlap the names, the margin should be a bit smaller and should depend on the line height, not the maximum character width, and the scaling of the bubble size with distance probably needs some love as well. But I got the basic feature working: speech_bubble The reason the bubble itself was not drawn was interesting: the winding direction of the polygon was wrong. Now, I distinctly remember seeing the speech bubbles before (a long, long time ago), but if this ever worked with how it was coded in the current tree, it must have been entirely by accident.

pjbroad commented 3 years ago

I feel much better now that I could not work out how to fix the previous code either, I also considered just using a more standard approach so I think that's fine. Interesting about the background as I too remember it working at some stage and the code is unchanged for rather a long time. There are some unexpected effects with text size for multiple characters with the same settings but I'll wait for you to finish adjusting. Thanks again for looking at this.

gvissers commented 3 years ago

Ok, I have taken it as far as I can without further input. I am not entirely happy with the result, but then again, I have never been entirely happy with the speech bubbles :smile: Perhaps the bubbles are now a bit high above the actor, but if we lower them they overlap with the background of the player name for users that have enabled alpha backgrounds.

Let me hear what you think,and if it needs any improvements.

pjbroad commented 3 years ago

I think that's fine, much better than I remember the feature working before. I like the text fixed to name position and it works fine in instance mode etc. The only think I'd suggest changing would be perhaps allowing more text rather than truncating, perhaps using multiple lines of text as needed.

gvissers commented 3 years ago

The only think I'd suggest changing would be perhaps allowing more text rather than truncating, perhaps using multiple lines of text as needed.

Yeah, the truncation was kept from the existing code. I will probably take a look at it later, see if we can split it into lines instead.

gvissers commented 3 years ago

The only think I'd suggest changing would be perhaps allowing more text rather than truncating, perhaps using multiple lines of text as needed.

Yeah, the truncation was kept from the existing code. I will probably take a look at it later, see if we can split it into lines instead.

Done.

pjbroad commented 3 years ago

Much better. A few points.

gvissers commented 3 years ago

Much better. A few points.

* The maximum length is four character short, perhaps due to the text wrapping?  Also there is a #define in chat.h (MAX_TEXT_MESSAGE_LENGTH) that sets the maximum length for input that we should probably use.

Yeah that's probably due to the newlines inserted by the wrapping, we should increase the buffer size a bit. I had already tried to use the define, but that leads to a circular include dependency actors.h -> chat.h -> actors.h. I'll see if I can reasonably move the define to a common shared include file, or perhaps move the inclusion of actors.h in chat.h to after the definition of MAX_TEXT_MESSAGE_LENGTH.

* The wrapped text sometimes exceeds the bounds of the bubble background.

Right, that shouldn't happen, will investigate. Is it only in the corners, or does it really go over the side of the bubble?

* The text is obscured by objects at some zoom levels.  I'm sitting in Portland storage while testing.

That is by design. I can draw the bubbles on top, but IMHO if the corresponding actor is occluded, those disassociated bubbles look very strange. Besides, I don't imagine people use the overhead text as their primary method of following local chat. But if you prefer, I can move the bubbles up front.

pjbroad commented 3 years ago

Wow, I wish I'd not mentioned the #define now. The text exceeding the bubble background is not just the corners. I was testing use this text "The enhanced actor structure holds information about the actors extensions such as if the actor is wearing any armour, weapons etc. abcdefghijklmnopqrstuvwxyz01". This text "dfgdfg dfgdfgd" also reaches the border of the bubble. It happens with my chosen TTF font but also with the font 0. It looks to be scale dependant and I see the problem with a scale of 1.3.

gvissers commented 3 years ago

Wow, I wish I'd not mentioned the #define now.

I agree that it was a bit more reorganization than the use single #define might warrant. But I also think it has made the code a bit more robust: the check_harvesting_effect() function had nothing at all to do with text handling, code that only needs to process text no longer needs to include all of the eye candy headers. Similarly, the map editor does not use actors, getting rid of the unneeded dependency allows us to change code in that area more easily without breaking the map editor.

It looks to be scale dependant and I see the problem with a scale of 1.3.

Thanks for the debug info, I think I have found the problem. Rounding of individual character widths when scaling a text leads to the total width not being simply equal to scaling the original width. I did think of this before, but did not expect the effect to be so large. Oh well. It was not necessarily scale dependant, I think the problem might have occurred with other actors at different distances as well, even if the font scaling factor was an integer.

We may still need to decide de depth at which we draw the text bubbles. As is, they are obscured by objects in front of the bubble, which can be annoying. Drawing them on top of everything can make bubbles appear for actors that are obscured, which looks strange IMHO. On the other hand, you might be less likely to miss what someone is saying. I don't know which option is less bad, let me know if you have a preference.

pjbroad commented 3 years ago

With the latest changes, this looks well fixed. Thanks for the work that you've put into fixing what appeared to be a simple bug. As for the depth, like you say, neither approach is completely satisfying though perhaps the way it is currently is more as you'd expect. I'd say leave it as it is and see if there are any comments. If you concur, we should close this issue.

gvissers commented 3 years ago

and see if there are any comments.

I highly doubt it :laughing:

If you concur, we should close this issue.

All right, closing.