gurkenlabs / litiengine

LITIENGINE 🕹 The pure 2D java game engine.
https://litiengine.com/
MIT License
725 stars 93 forks source link

Sometimes automatic line breaks start at the wrong x-position #474

Closed Hades948 closed 4 months ago

Hades948 commented 1 year ago

Describe the bug Sometimes automatic line breaks start at the wrong x-position. For example, if I set the text of a GuiComponent to be "This is a much longer test to test text box wrapping for the NPC text boxes." it looks like this: image

Clearly not what it's supposed to look like. However, if I change nothing other than the string, it works. For example, if I replace the 'g' in "wrapping" to a period, it looks fine: image

So, I'm guessing that first line just happened to be juuuust the right width to break something.

In Align.getLocation, the objectWidth > width for the bad string, but < width for the good string.

Because of this, location is negative but doesn't get clamped. I'm guessing that's the issue. Or at least roughly in that area.

To Reproduce Steps to reproduce the behavior:

Your System:

nightm4re94 commented 1 year ago

I can reproduce this, but couldn't pinpoint the source of this issue. What I can say for sure is that it depends on the font used due to the differences in glyph width: image

Hades948 commented 1 year ago

I recently added UI resizing and I've noticed that I can get it to reliably happen for all of my text boxes, just at different sizes. I think the textbox size that it happens at changes for different fonts/strings. I took this GIF with just whatever the default Java font is when you do new Font(null, Font.PLAIN, 28): litiengine_issue_474 I also recreated this issue with the "Monospaced" font (Using "Monospaced" instead of null in the Font constructor).

Gamebuster19901 commented 1 year ago

I remember also having issues with text rendering in java in one of my own projects.

I had to render the text using a GlyphVector to calculate bounds and stuff because it would just be wrong very often, however I've never seen it wrong by that much.

Just from a visual glance, it looks like the text's x-coordinate is being set to -(w*0.5) where w is the width of the line of text being rendered.

hedfol commented 4 months ago

Hi LITIENGINE community,

Can I make my first contribution by working on this issue?

I discovered the same cause that was already mentioned in a bug description: negative x position is produced by Align.getLocation(width, objectWidth) when a string line width (objectWidth) exceeds available space (width). This affects not just the LEFT but all values of Align. Similar issues occur with Valign enum:

gui-comp-text-overflow png

If these lines are removed:

https://github.com/gurkenlabs/litiengine/blob/84d617a2e556b438f0a526370359c64db398382b/litiengine/src/main/java/de/gurkenlabs/litiengine/Align.java#L93-L95

https://github.com/gurkenlabs/litiengine/blob/84d617a2e556b438f0a526370359c64db398382b/litiengine/src/main/java/de/gurkenlabs/litiengine/Valign.java#L92-L94

position gets clamped:

gui-comp-text-overflow-clamp

However, two tests fail:

AlignTests > getLocation_InPoint(): expected: <-1.5> but was: <0.0>
AlignTests > getLocation_OffPoint(): expected: <0.45> but was: <0.0>
Test code:

https://github.com/gurkenlabs/litiengine/blob/84d617a2e556b438f0a526370359c64db398382b/litiengine/src/test/java/de/gurkenlabs/litiengine/AlignTests.java#L25-L41

Both alignment enums are used not only in GuiComponent.renderText() (related to this issue) but also in PhysicsEngine, CollisionEntity, and direct calls of TextRenderer methods. Is it necessary to separate text and entity alignments? For example: create a dedicated TextAlign enum, or just use a common Align but rename getLocation() to getTextLocation() and add getEntityLocation().

Either way an expected location behavior should be determined for all alignment values.

nightm4re94 commented 4 months ago

Hi @hedfol , thank you for your detailed assessment! We welcome your contribution in this regard. I would neither create separate classes nor methods for this, but an overload to the getLocation method with an additional boolean parameter that lets the user decide which behaviour to choose. Since the current logic seems to be assuming the entity case, we should leave the uses of getLocation in entities untouched and only replace the uses in text scenarios with the new overload.

hedfol commented 4 months ago

Thanks @nightm4re94 , that's an easy solution! I've created a PR with suggested changes.