0002048: reloading game causes crash if there was a path visible

(0005445)
SXX (administrator)
2015-02-10 22:43

This one really annoying and likely have multiple duplicates, e.g: 0001771, 0001672 and likely many more include some my crash report.

I also guess this problem have same nature with other crash inside "CPathfinder::calculatePaths" on loading maps as those two likely use same features.

(0005453)
Fay (developer)
2015-02-13 10:24

Fix in this PR seems to fix the problem for me. https://github.com/vcmi/vcmi/pull/87 [^]

Haven't checked other reports, but it's likely that these are the same issues.

(0005455)
Fay (developer)
2015-02-13 11:05

Should I add 'related to' for these other similar issues and/or add notes about possibly being fixed there?

(0005456)
Warmonger (administrator)
2015-02-13 11:15

Maybe, but after merge I can reproduce the issue just the same :/

(0005457)
Fay (developer)
2015-02-13 11:40

Strange. I was able to reproduce it nearly 100% of time and after that fix I no longer get this crash.

However, I've just found another problem related to reloading while testing now. When I reload between 2 saves (not any saves, just these two), it crashes 100% of time because of broken tile surface ptr. Not related to paths.

Does your crash point to showPath()?

(0005458)
SXX (administrator)
2015-02-13 12:32

I'm sorry, but are you sure that setting nullptr there is actually correct fix for this problem?

Of course it's fix the crash itself, but is that pointer have to be there after loading at all? Isn't this kind a hacky solution?

(0005459)
SXX (administrator)
2015-02-13 12:36
edited on: 2015-02-13 12:37

What I mean: there is multiple similar problems where game state incorrect after exiting to menu and back to game, e.g one with "CPathfinder::calculatePaths" where pointer's aren't the same. So I just worry there is some bigger flaw somewhere around and only fixing it's consequences may be not that good idea.

So I wonder may be there have to be feature that restore client state to the point like it's was just after startup to menu. Or something like that.

(0005460)
Fay (developer)
2015-02-13 13:08

This crash is/was caused by the same problem as 0001953. When you reload the game (load a save after playing a map), player interface reuses CAdvMapInt instance instead of creating new one.

Current path isn't set until it's created by user or deserialized from save state (later I think). That's why after reload, terrain.currentPath points to "path" from previous game.

(0005461)
Fay (developer)
2015-02-13 13:10

The other solution would be to recreate adv map interface every time you start the game. I don't know if there's a valid reason to keep and reuse an instance (probably high cost of creating new one?).

(0005462)
Warmonger (administrator)
2015-02-13 13:35

It's easier to manage if you create new interface every time. This can't be costly as you do that on every new game anyway.

(0005463)
SXX (administrator)
2015-02-13 14:43

I think it's better solution to create as much as technically possible from scratch to avoid such conflicts. Crashes like this one are way too annoying and sometimes hard to debug so better to avoid them completely.

(0005465)
Fay (developer)
2015-02-13 15:42

I can try changing it (probably not today though).
But still, I'm not sure if it's safe to recreate the instance; sounds risky. ;)

(0005467)
AVS (administrator)
2015-02-13 16:26

If we decide to continue reusing advMapInt objects, then we should once more review CAdvMapInt::restoreState().

(0005468)
AVS (administrator)
2015-02-13 16:39

And I also don`t like the existence of terrain.currentPath (as variable) at all. This will surely cause more crashes in future.

Issue History
Date Modified	Username	Field	Change
2015-01-19 23:39	Fay	New Issue
2015-01-19 23:39	Fay	Status	new => assigned
2015-01-19 23:39	Fay	Assigned To	=> Tow
2015-02-10 22:43	SXX	Note Added: 0005445
2015-02-13 10:24	Fay	Note Added: 0005453
2015-02-13 10:24	Fay	Assigned To	Tow => Fay
2015-02-13 11:03	Fay	Status	assigned => resolved
2015-02-13 11:03	Fay	Resolution	open => fixed
2015-02-13 11:03	Fay	Fixed in Version	=> 0.97c
2015-02-13 11:05	Fay	Note Added: 0005455
2015-02-13 11:15	Warmonger	Note Added: 0005456
2015-02-13 11:40	Fay	Note Added: 0005457
2015-02-13 12:32	SXX	Note Added: 0005458
2015-02-13 12:36	SXX	Note Added: 0005459
2015-02-13 12:37	SXX	Note Edited: 0005459	View Revisions
2015-02-13 13:08	Fay	Note Added: 0005460
2015-02-13 13:10	Fay	Note Added: 0005461
2015-02-13 13:35	Warmonger	Note Added: 0005462
2015-02-13 14:43	SXX	Note Added: 0005463
2015-02-13 15:42	Fay	Note Added: 0005465
2015-02-13 16:26	AVS	Note Added: 0005467
2015-02-13 16:39	AVS	Note Added: 0005468
2015-11-18 03:15	SXX	Relationship added	related to 0001954
2016-11-11 08:22	SXX	Status	resolved => closed

Notes
(0005445) SXX (administrator) 2015-02-10 22:43	This one really annoying and likely have multiple duplicates, e.g: 0001771, 0001672 and likely many more include some my crash report. I also guess this problem have same nature with other crash inside "CPathfinder::calculatePaths" on loading maps as those two likely use same features.

(0005453) Fay (developer) 2015-02-13 10:24	Fix in this PR seems to fix the problem for me. https://github.com/vcmi/vcmi/pull/87 [^] Haven't checked other reports, but it's likely that these are the same issues.

(0005455) Fay (developer) 2015-02-13 11:05	Should I add 'related to' for these other similar issues and/or add notes about possibly being fixed there?

(0005456) Warmonger (administrator) 2015-02-13 11:15	Maybe, but after merge I can reproduce the issue just the same :/

(0005457) Fay (developer) 2015-02-13 11:40	Strange. I was able to reproduce it nearly 100% of time and after that fix I no longer get this crash. However, I've just found another problem related to reloading while testing now. When I reload between 2 saves (not any saves, just these two), it crashes 100% of time because of broken tile surface ptr. Not related to paths. Does your crash point to showPath()?

(0005458) SXX (administrator) 2015-02-13 12:32	I'm sorry, but are you sure that setting nullptr there is actually correct fix for this problem? Of course it's fix the crash itself, but is that pointer have to be there after loading at all? Isn't this kind a hacky solution?

(0005459) SXX (administrator) 2015-02-13 12:36 edited on: 2015-02-13 12:37	What I mean: there is multiple similar problems where game state incorrect after exiting to menu and back to game, e.g one with "CPathfinder::calculatePaths" where pointer's aren't the same. So I just worry there is some bigger flaw somewhere around and only fixing it's consequences may be not that good idea. So I wonder may be there have to be feature that restore client state to the point like it's was just after startup to menu. Or something like that.

(0005460) Fay (developer) 2015-02-13 13:08	This crash is/was caused by the same problem as 0001953. When you reload the game (load a save after playing a map), player interface reuses CAdvMapInt instance instead of creating new one. Current path isn't set until it's created by user or deserialized from save state (later I think). That's why after reload, terrain.currentPath points to "path" from previous game.

(0005461) Fay (developer) 2015-02-13 13:10	The other solution would be to recreate adv map interface every time you start the game. I don't know if there's a valid reason to keep and reuse an instance (probably high cost of creating new one?).

(0005462) Warmonger (administrator) 2015-02-13 13:35	It's easier to manage if you create new interface every time. This can't be costly as you do that on every new game anyway.

(0005463) SXX (administrator) 2015-02-13 14:43	I think it's better solution to create as much as technically possible from scratch to avoid such conflicts. Crashes like this one are way too annoying and sometimes hard to debug so better to avoid them completely.

(0005465) Fay (developer) 2015-02-13 15:42	I can try changing it (probably not today though). But still, I'm not sure if it's safe to recreate the instance; sounds risky. ;)

(0005467) AVS (administrator) 2015-02-13 16:26	If we decide to continue reusing advMapInt objects, then we should once more review CAdvMapInt::restoreState().

(0005468) AVS (administrator) 2015-02-13 16:39	And I also don`t like the existence of terrain.currentPath (as variable) at all. This will surely cause more crashes in future.