Fixing text buffer texture/font corruption (#479) #534

2025-01-16T18:17:21Z

NQNStudios commented

2025-01-16 18:17:21 +00:00

(Migrated from github.com)

Debugging #479 has become a meandering process, with some other bugs needing to be fixed along the way. Might as well keep them in 1 PR.

#479 demonstrates that the contents of the text buffer are NOT irrelevant for reproducing bugs. So I set up recording/replay for the burma shave easter egg. This also makes an easy way to mess with the buffer state when debugging (just mash &/*/&/*/&/* n times)
When a replay throws an error, it puts up a showError() dialog. If the next action is a control_click, the system will try to click that control on the error dialog--which is totally divergent from the replay's intended behavior. So we should just stop replaying when an error happens.
If you have a long replay and want to run it very fast, but then slow down when you get to the sequence that reproduces your bug, now you can add a <change_fps> to your replay to achieve that.
Fixes for the 2 legacy replay errors that I opened yesterday

I still am quite stumped on the real error for #479. I asked for help in the SFML Discord channel, here's a link to that thread (you'll need a discord account and to join the SFML group to see it). But anyway, this link makes it easy for me to consult back to what people suggested.

EDIT: Changed the title to clarify one thing I do know about this bug: the text buffer array still contains the correct text. This is a case of the sf::Text that's rendered onto text_area_gworld being corrupt somehow. Maybe the font gets corrupted.

Fix #479
Fix #532
Fix #533

Debugging #479 has become a meandering process, with some other bugs needing to be fixed along the way. Might as well keep them in 1 PR. * #479 demonstrates that the contents of the text buffer are NOT irrelevant for reproducing bugs. So I set up recording/replay for the burma shave easter egg. This also makes an easy way to mess with the buffer state when debugging (just mash &/\*/&/\*/&/\* n times) * When a replay throws an error, it puts up a showError() dialog. If the next action is a control_click, the system will try to click that control on the error dialog--which is totally divergent from the replay's intended behavior. So we should just stop replaying when an error happens. * If you have a long replay and want to run it very fast, but then slow down when you get to the sequence that reproduces your bug, now you can add a `<change_fps>` to your replay to achieve that. * Fixes for the 2 legacy replay errors that I opened yesterday I still am quite stumped on the real error for #479. I asked for help in the SFML Discord channel, here's a [link to that thread](https://discord.com/channels/175298431294636032/175298431294636032/1329466205366390825) (you'll need a discord account and to join the SFML group to see it). But anyway, this link makes it easy for me to consult back to what people suggested. EDIT: Changed the title to clarify one thing I do know about this bug: the text buffer array still contains the correct text. This is a case of the `sf::Text` that's rendered onto text_area_gworld being corrupt somehow. Maybe the font gets corrupted. Fix #479 Fix #532 Fix #533

NQNStudios commented

2025-01-16 23:02:38 +00:00

(Migrated from github.com)

@CelticMinstrel This bug was caused by this line:

62ddf3d0b3 (diff-0352715fc63f00d5823113f53777233ac00d70eb82df14dc28b2e9366710ee86L961)

When creating a new PC, the empty slot was being filled by this cPlayer constructor which set the player name to "\n". Then, pick_pc_name() would be called, putting the "\n" in as the text field's default value.

Now, I don't know why, but it appears that the larger issue here is that if we accidentally try to render a newline character, the font object becomes borked. This probably explains #454 (one instance of which this PR also fixes) and maybe #417 as well. We need to find all the ways we might end up trying to do that... And maybe add an assertion that there are no newlines in strings being drawn.

In this case, setting the default player name to empty string, fixes #479 as reproduced by the log in that thread.

Was there any reason to set the player's default name to a newline and not an empty string?

@CelticMinstrel This bug was caused by this line: https://github.com/calref/cboe/pull/534/commits/62ddf3d0b3f1baf7b2b9330d732832957f7b1c5b#diff-0352715fc63f00d5823113f53777233ac00d70eb82df14dc28b2e9366710ee86L961 When creating a new PC, the empty slot was being filled by this cPlayer constructor which set the player name to "\n". Then, pick_pc_name() would be called, putting the "\n" in as the text field's default value. Now, I don't know why, but it appears that the larger issue here is that if we accidentally try to render a newline character, the font object becomes borked. This probably explains #454 (one instance of which this PR also fixes) and maybe #417 as well. We need to find all the ways we might end up trying to do that... And maybe add an assertion that there are no newlines in strings being drawn. In this case, setting the default player name to empty string, fixes #479 as reproduced by the log in that thread. Was there any reason to set the player's default name to a newline and not an empty string?

NQNStudios commented

2025-01-17 01:00:43 +00:00

(Migrated from github.com)

According to the blame, it looks like the "\n" default name has been in place since 16 years ago when Mac and Windows code were separate. I'm thinking its purpose is obsolete and an empty string is fine?

CelticMinstrel commented

2025-01-17 14:43:25 +00:00

(Migrated from github.com)

According to the blame, it looks like the "\n" default name has been in place since 16 years ago when Mac and Windows code were separate. I'm thinking its purpose is obsolete and an empty string is fine?

I found the "\n" in the original source code released by Spiderweb Software - at party.c:528. I have no idea why it was there, but it used to be an sprintf call. If you used blame I'm guessing you saw this as well.

Now, I don't know why, but it appears that the larger issue here is that if we accidentally try to render a newline character, the font object becomes borked.

This strikes me as kind of weird. Are we doing something strange in our rendering? I don't think this happens in other projects where I use SFML – rendering a newline should just be a no-op. Though admittedly, other projects probably only render a newline as part of a run of text and not alone… or it's possible I had logic to split on newline so the newline actually isn't being rendered.

> According to the blame, it looks like the "\n" default name has been in place since 16 years ago when Mac and Windows code were separate. I'm thinking its purpose is obsolete and an empty string is fine? I found the `"\n"` in the original source code released by Spiderweb Software - at party.c:528. I have no idea why it was there, but it used to be an `sprintf` call. If you used blame I'm guessing you saw this as well. > Now, I don't know why, but it appears that the larger issue here is that if we accidentally try to render a newline character, the font object becomes borked. This strikes me as kind of weird. Are we doing something strange in our rendering? I don't think this happens in other projects where I use SFML – rendering a newline should just be a no-op. Though admittedly, other projects probably only render a newline as part of a run of text and not alone… or it's possible I had logic to split on newline so the newline actually isn't being rendered.

NQNStudios commented

2025-01-17 16:16:43 +00:00

(Migrated from github.com)

I’ll dig into the nitty gritty of why it happens—we should probably know
exactly why.

On Fri, Jan 17, 2025 at 8:43 AM Celtic Minstrel @.***>
wrote:

According to the blame, it looks like the "\n" default name has been in
place since 16 years ago when Mac and Windows code were separate. I'm
thinking its purpose is obsolete and an empty string is fine?

I found the "\n" in the original source code released by Spiderweb
Software - at party.c:528. I have no idea why it was there, but it used to
be an sprintf call. If you used blame I'm guessing you saw this as well.

Now, I don't know why, but it appears that the larger issue here is that
if we accidentally try to render a newline character, the font object
becomes borked.

This strikes me as kind of weird. Are we doing something strange in our
rendering? I don't think this happens in other projects where I use SFML –
rendering a newline should just be a no-op. Though admittedly, other
projects probably only render a newline as part of a run of text and not
alone… or it's possible I had logic to split on newline so the newline
actually isn't being rendered.

—
Reply to this email directly, view it on GitHub
https://github.com/calref/cboe/pull/534#issuecomment-2598520347, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AATXBKKLSVZYQ3EJBY2STRT2LEJKJAVCNFSM6AAAAABVKHJUYKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJYGUZDAMZUG4
.
You are receiving this because you authored the thread.Message ID:
@.***>

I’ll dig into the nitty gritty of why it happens—we should probably know *exactly* why. On Fri, Jan 17, 2025 at 8:43 AM Celtic Minstrel ***@***.***> wrote: > According to the blame, it looks like the "\n" default name has been in > place since 16 years ago when Mac and Windows code were separate. I'm > thinking its purpose is obsolete and an empty string is fine? > > I found the "\n" in the original source code released by Spiderweb > Software - at party.c:528. I have no idea why it was there, but it used to > be an sprintf call. If you used blame I'm guessing you saw this as well. > > Now, I don't know why, but it appears that the larger issue here is that > if we accidentally try to render a newline character, the font object > becomes borked. > > This strikes me as kind of weird. Are we doing something strange in our > rendering? I don't think this happens in other projects where I use SFML – > rendering a newline should just be a no-op. Though admittedly, other > projects probably only render a newline as part of a run of text and not > alone… or it's possible I had logic to split on newline so the newline > actually isn't being rendered. > > — > Reply to this email directly, view it on GitHub > <https://github.com/calref/cboe/pull/534#issuecomment-2598520347>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AATXBKKLSVZYQ3EJBY2STRT2LEJKJAVCNFSM6AAAAABVKHJUYKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOJYGUZDAMZUG4> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

NQNStudios commented

2025-01-18 19:51:22 +00:00

(Migrated from github.com)

I've modified draw_startup() to just directly do some tests.

void draw_startup(short but_type) {
	
        // We can change this line to demonstrate what breaks the normal text declared below
        sf::Text text("\n", *ResMgr::fonts.get("plain"), 12);


	mainPtr.draw(text);
	sf::Text text2("This text should appear normal", *ResMgr::fonts.get("plain"), 12);
	text2.setPosition(100, 100);
	mainPtr.draw(text2);
}

This first version, just putting "\n" in an sf::Text, doesn't break anything.

A newline in the middle of a string also doesn't break anything. Actually, SFML handles the newline.

sf::Text text("text\nwith a newline in it", *ResMgr::fonts.get("plain"), 12);

Now, using our cboe library functions:

	rectangle dest = { 0, 0, 50, 400};
	TextStyle style;	
	win_draw_string(mainPtr,dest,"\n",eTextMode::LEFT_TOP,style);

Still good.

win_draw_string with a newline in the middle:

Now I'm getting surprised this all still works.

#479 was caused by the newline being in the string for a cTextField, and those use draw_string_hilite so I tried that next:

	draw_string_sel(mainPtr,dest,"\n",style,{{0, 1}}, Colours::BLUE);
        // OR
        draw_string_hilite(mainPtr,dest,"\n",style,{{0, 1}}, Colours::BLUE);

both give the same result:

And still not reproducing the bug.

Still getting to the bottom of this.

I've modified draw_startup() to just directly do some tests. ``` void draw_startup(short but_type) { // We can change this line to demonstrate what breaks the normal text declared below sf::Text text("\n", *ResMgr::fonts.get("plain"), 12); mainPtr.draw(text); sf::Text text2("This text should appear normal", *ResMgr::fonts.get("plain"), 12); text2.setPosition(100, 100); mainPtr.draw(text2); } ``` This first version, just putting "\n" in an sf::Text, doesn't break anything. <img width="334" alt="Screenshot 2025-01-18 at 1 33 28 PM" src="https://github.com/user-attachments/assets/a1d82133-4760-4f2b-aa10-f74d7d8e4762" /> A newline in the middle of a string also doesn't break anything. Actually, SFML handles the newline. `sf::Text text("text\nwith a newline in it", *ResMgr::fonts.get("plain"), 12);` <img width="454" alt="Screenshot 2025-01-18 at 1 34 42 PM" src="https://github.com/user-attachments/assets/ecffcbd9-feab-4bd5-8df6-53e0c49ffe09" /> Now, using our cboe library functions: ``` rectangle dest = { 0, 0, 50, 400}; TextStyle style; win_draw_string(mainPtr,dest,"\n",eTextMode::LEFT_TOP,style); ``` <img width="328" alt="Screenshot 2025-01-18 at 1 41 33 PM" src="https://github.com/user-attachments/assets/cd7ca6bf-0c65-4898-b37b-d88857938c44" /> Still good. win_draw_string with a newline in the middle: <img width="468" alt="Screenshot 2025-01-18 at 1 43 12 PM" src="https://github.com/user-attachments/assets/f0136139-4f3c-45be-9907-7ab042141606" /> Now I'm getting surprised this all still works. #479 was caused by the newline being in the string for a cTextField, and those use draw_string_hilite so I tried that next: ``` draw_string_sel(mainPtr,dest,"\n",style,{{0, 1}}, Colours::BLUE); // OR draw_string_hilite(mainPtr,dest,"\n",style,{{0, 1}}, Colours::BLUE); ``` both give the same result: <img width="438" alt="Screenshot 2025-01-18 at 1 50 13 PM" src="https://github.com/user-attachments/assets/b20e92bf-04a2-4431-a80f-e513da6b81bd" /> And still not reproducing the bug. Still getting to the bottom of this.

NQNStudios commented

2025-01-18 20:02:48 +00:00

(Migrated from github.com)

No more messing around, I tried just launching a text field with "\n" in it:

	static bool ran_dialog = false;
	if(!ran_dialog){
		cDialog pcPickName(*ResMgr::dialogs.get("pick-pc-name"), nullptr);
		pcPickName["name"].setText("\n");
		pcPickName["okay"].attachClickHandler([](cDialog& me, std::string _1, eKeyMod _2) -> bool {
			me.toast(true);
			return true;
		});
		
		pcPickName.run();
		ran_dialog = true;
	}

But this still didn't break anything!

No more messing around, I tried just launching a text field with "\n" in it: ``` static bool ran_dialog = false; if(!ran_dialog){ cDialog pcPickName(*ResMgr::dialogs.get("pick-pc-name"), nullptr); pcPickName["name"].setText("\n"); pcPickName["okay"].attachClickHandler([](cDialog& me, std::string _1, eKeyMod _2) -> bool { me.toast(true); return true; }); pcPickName.run(); ran_dialog = true; } ``` But this still didn't break anything!

NQNStudios commented

2025-01-18 20:20:07 +00:00

(Migrated from github.com)

I still honestly don't know how to reproduce this bug without running the very long replay that entails printing to the buffer hundreds of times and doing all kinds of weird state changes. But even then it doesn't feel like it comes from a random memory leak, because Windows and Mac both share the problem (albeit presented differently visually).

NQNStudios commented

2025-01-18 20:56:57 +00:00

(Migrated from github.com)

Could the newline in the default name have been put there to avoid a segfault/out-of-bounds here?

3daf1d55e4/src/game/boe.party.cpp (L2225)

Could the newline in the default name have been put there to avoid a segfault/out-of-bounds here? https://github.com/calref/cboe/blob/3daf1d55e42abb4f9118f98f03a2de1cebda1b5f/src/game/boe.party.cpp#L2225

CelticMinstrel commented

2025-01-20 14:15:00 +00:00

(Migrated from github.com)

Could the newline in the default name have been put there to avoid a segfault/out-of-bounds here?

Seems plausible, but that wouldn't be a segfault even on an empty string, since strings need to be null-terminated (even std::string needs that so that c_str() works), and also because a short string doesn't allocate any memory (there's no heap pointer to dereference). It would probably be an out-of-bounds assertion in some debug C++ runtimes though.

> Could the newline in the default name have been put there to avoid a segfault/out-of-bounds here? Seems plausible, but that wouldn't be a segfault even on an empty string, since strings need to be null-terminated (even `std::string` needs that so that `c_str()` works), and also because a short string doesn't allocate any memory (there's no heap pointer to dereference). It would probably be an out-of-bounds assertion in some debug C++ runtimes though.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: openboe/oboe#534