Fixed dungeon generation (by retrying), fixable sumo house (thru DevCons cmd), etc.

[AquariusPower]

Fixed dungeon generation about "invalid config sought" that would ABORT;
Added a dev cmd to fix the over crowded sumo house;
Added some command line commands, environment variables and a --help command, mainly for developers;
The environment variable can be used to test the dungeon generation many times, default is 250 times.
Fixed crafting a item that breaks and has no BROKEN config;
Fixed developer console to use festring and more reliable MACROS;
Added many new commands to developer console;

all copied from #587

[ryfactor]

I've been wondering about this strategy and feared that re-generating dungeon level might lead to memory leaks. This might be due Alloc2D calls in level::Generate etc in level.cpp. We will need to have some means of freeing the memory if the level had not been generated successfully, before moving on to trying again.

[AquariusPower]

If I remember it well, when a new dungeon level is loaded, the old one is just deleted right? it auto deletes everything inside thru destructors I think. And that doesnt happen so often in a normal gameplay.
But... when put in such loop, for the 250 retry test, after about may be 50 retries you start seeing the difference in memory usage MB by the game.

So, just to fix a normal gameplay, the benefit is better then the memory leak, but that is indeed still a problem. I believe this problem was already present but I can't be sure without tests in a normal gameplay (or may be wizard auto-play) and it would be good to add some counter to know how many times dungeon levels got deleted (it could just output to stderr). If this problem wasn't happening, then to focus on this small retry code could solve it :).

Alloc2D() is used with 2 variables that I think there could have something missing in some destructor, not necessarily on the below code but may be something deeper:

level::~level()
{
  ulong c;

  for(c = 0; c < XSizeTimesYSize; ++c)
    delete NodeMap[0][c];

  for(c = 0; c < Room.size(); ++c)
    delete Room[c];

  delete [] NodeMap;
  delete [] WalkabilityMap;

ivan/Main/Source/level.cpp

Line 41 in 87f4a33

level::~level()

[AquariusPower]

I think a profiler can detect and pin point memory leaks in the code right?
The only one I saw running and was very good was from netbeans, but I think it was a java application, despite I think it can profile C++ apps too (I use netbeans to help codign but I compile it manually as I wasnt able yet to make it compile thru netbeans). There is one profiler for eclipse too, but I had a lot of trouble trying to make it run in the past (dont even remember if it woked at all). Well any profiler would do I guess.

[ryfactor]

Alloc2D() is used with 2 variables that I think there could have something missing in some destructor, not necessarily on the below code but may be something deeper:

I agree, it probably lies deeper, as it is in all good dungeons...! There's bound to be some extraneous factor leading to leaks elsewhere.

In benign circumstances, even if the problem is around 1 in 10 games, and the game successfully regenerates a dungeon on the second try, then it is pretty decent. By the time you get to regenerate 50 times, it is probably 1:10^12 games or soemthing like that.

All the same, it might be good to have a generation counter, as you suggest.

[ryfactor]

Re. the profiler, I think valgrind on linux, or similarly the Tracy profiler (cross-platform). The Tracy profiler manual is on the release page: https://github.com/wolfpld/tracy/releases

N.B. Tracy profiler makes use of an immediate-mode gui (Dear ImGui by @ocornut) to present results. This makes it very responsive and easy to read.

[AquariusPower]

cool, I will try Tracy,

btw, I tested with valgrind
valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose --log-file=valgrind-out.txt ./ivan
the game in wizard mode,
I teleported "|" to some levels to let them be deleted, about 4 times.
but on the log, there is no destructors... no "::~" to be found... so I cant relate memoryleak to a dungeon level deletion from this log...

this was the log summary for that short run:

==31254== LEAK SUMMARY:
==31254==    definitely lost: 5,505 bytes in 165 blocks
==31254==    indirectly lost: 13,132,236 bytes in 55,934 blocks
==31254==      possibly lost: 5,103,400 bytes in 7,672 blocks
==31254==    still reachable: 16,509,246 bytes in 7,504 blocks
==31254==                       of which reachable via heuristic:
==31254==                         length64           : 96 bytes in 6 blocks
==31254==                         newarray           : 162,144 bytes in 1,143 blocks
==31254==         suppressed: 0 bytes in 0 blocks
==31254== 
==31254== ERROR SUMMARY: 10000230 errors from 266 contexts (suppressed: 6 from 1)

On the full log, there seems to have memory leaks everywhere!
And I think I may have coded one of the biggest ones (at least on a short run) at "main.pp SkipGameScript()" when it analyses the savegame files to gather info to show on the continue game list. I thought stuff I collected were being auto deleted on their destructors, may be I was wrong? may be they just point to data/memory instead of holding the data?

[AquariusPower]

mmm... I am having some trouble to compile Tracy on ubuntu, I think it is dependencies, will try again later.
yep seems quite difficult to compile on linux wolfpld/tracy#34 ... a pity, the screenshot is amazing!

[ryfactor]

@AquariusPower these DevCons commands are quite cool. I've been playing around with generating the dungeons, and they seem to generate normally. Do you have a savefile with bad dungeon generation I can test?
I still need to get around to testing the Sumo House fix.

[AquariusPower]

@AquariusPower these DevCons commands are quite cool. I've been playing around with generating the dungeons, and they seem to generate normally. Do you have a savefile with bad dungeon generation I can test? I still need to get around to testing the Sumo House fix.

I think I may not have documented this well, the below steps should be in the developer documentation!

To test "retry dungeon creation" run this:
IVAN_DebugGenDungeonLevelLoopID=43 IVAN_DebugGenDungeonLevelLoopMax=500 ./ivan

• start a new game
• enter the first dungeon cave
• enter wizard mode
• teleport to crystal caves (type "|" "4")

It will start to recreate that dungeon 500 times in a loop, to let help it crash randomly.
It will exit the game as soon the test ends
To see how many times it retried already run this tail -F ~/.ivan/.ivan.dbgmsg.log| grep "fsGenLoopDL" (TODO: these retries could be shown on the terminal tho with printf)
Beware, the 500 loop test will require 1.6GB free RAM because of the memory leak bug when deleting/de-allocating a dungeon from RAM memory.

Obs.: that dungeon level was created 500 times w/o a problem right now! what is good news!

[ryfactor]

When testing FixSumoHouse with the situation depicted in the accompanying image, it doesn't fix. Might need to get the algo to teleport any BG standing in the doorway as well.
To get bananagrower to stand in the doorway, I got the player character to stand in the doorway first, eat pineapples up to bloated, wait for bananagrower to come, eat some more pineapple, then rUn to the downstairs and challenge Sumo when BG is standing in doorway about to deliver banana. Then win fight, come back and everybody stuck in Sumo house.

I wonder if it would be easier to drop all the spectators and combatants outside Sumo house and then the whole thing might be moot. Or maybe only allow the sumo fighting if there are no bananagrowers in the sumo house including the doorway.

[ryfactor]

I think setting FeedingSumo = false; for all bananagrowers on the level immediately after a Sumo wrestling match might cause them to drop their bananas in the bananadroparea. See:

ivan/Main/Source/human.cpp

Line 3457 in e91cdad

cv2 BananaTarget = FeedingSumo ? SUMO_ROOM_POS + v2(1, 2) : v2(45, 45);

[AquariusPower]

I think setting FeedingSumo = false; for all bananagrowers on the level immediately after a Sumo wrestling match might cause them to drop their bananas in the bananadroparea. See:

ivan/Main/Source/human.cpp

Line 3457 in e91cdad

cv2 BananaTarget = FeedingSumo ? SUMO_ROOM_POS + v2(1, 2) : v2(45, 45);

If i remember it well, I think the workaround teleports the workers near the player, may be increasing the range could improve it also, another way would be to just teleport all workers everywhere. But the proper fix you suggest is much better overall :)

[AquariusPower]

btw, I generated the glowing caves about 3000 times w/o a problem. If I am not wrong, I think I found (or someone else did) the place the bug happens and fixed or workarounded the code there to prevent the "crash" (ABORT), that's why the generation in loop test shows no problems. Anyway, as I remember, there is a small retry limit in case it ever happens again, I think it is 5 or 10 retries, in a normal gameplay.

[ryfactor]

@AquariusPower I made a PR here on your branch: AquariusPower#68 for the bananagrower fix

btw, I generated the glowing caves about 3000 times w/o a problem. If I am not wrong, I think I found (or someone else did) the place the bug happens and fixed or workarounded the code there to prevent the "crash" (ABORT), that's why the generation in loop test shows no problems. Anyway, as I remember, there is a small retry limit in case it ever happens again, I think it is 5 or 10 retries, in a normal gameplay.

Cool, I tried out your crystal cavern generation method as well and it works good. I'm happy with how it behaves.

[AquariusPower]

Might need to get the algo to teleport any BG standing in the doorway as well.

Yes, looks good too, in case your proper fix doesnt suffice.
Not only where the player is, but also at the sumo house door location, it could look for, not only workers, but also for spectators as there are more than the ones on screenshot, despite I think the land lord (I forgot his name, with 2 red whips) should be skipped.
Let me check the code, it is as I remember, a loop in all directions centered on player. I could just be an extra loop centered on sumo house door. And it would cover all problematic spots against workers and spectators/tourists overcrowding.

[AquariusPower]

https://github.com/Attnam/ivan/pull/597/files#diff-e5b9350766723578f4cecfd649c49124103519367bdf419f76f5541c35957d5cR5382
at FixSumoWrestlerHouse()
now I see the check is centered on sumowrestler* (SM, the old champion) and not the player...
loops at SM->GetNeighbourSquares()
I think we can use ->GetNeighbourSquares() centered on the door spot (may be that spot->GetNeighbourSquares())
and then teleport bananagrower* and the tourists.
Or just loop on all neighboursquares collected for each of the SM->GetNeighbourSquares() loop (overkill), but it is just a temporary workaround.
Anyway, all that just in case the fix you proposed doesnt work for some specific case (like the screenshot that never happened to me :))