How to debug a Not Responding program element

[Kas Ob. 121]

5 hours ago, Willicious said:

neither has produced anything which fixes the issue:

A lot being said above, and you need to start and fig deeper and learn why and how.

 

Anyway, this issue is also easy to fix, the cause is this declaration in GR32.pas

  PColor32Array = ^TColor32Array;
  TColor32Array = array [0..0] of TColor32; // pain in the back, and will trigger overflow check
  TArrayOfColor32 = array of TColor32;

And the fix or another slap work around without testing but again using my extra power 

procedure TRenderer.ApplyRemovedTerrain(X, Y, W, H: Integer);
type
  TTempHackColor32Arr = array [0.. MaxInt div SizeOf(TColor32) -1 ] of TColor32;
  PTempHackColor32Arr = ^TTempHackColor32Arr;
var
  //PhysicsArrPtr, TerrLayerArrPtr: PColor32Array;
  PhysicsArrPtr, TerrLayerArrPtr: PTempHackColor32Arr;
  cx, cy: Integer;
  MapWidth: Integer; // Width of the total PhysicsMap
begin
  // This has two applications:
  // - Removing all non-solid pixels from rlTerrain (possibly created by blending)
  // - Removed pixels from PhysicsMap copied to rlTerrain (called when applying a mask from LemGame via RenderInterface)

  PhysicsArrPtr := PTempHackColor32Arr(PhysicsMap.Bits);
  TerrLayerArrPtr := PTempHackColor32Arr(fLayers[rlTerrain].Bits);

The best is to fix GR32 with the following, but don't do that on your own, this is up to Anders

  PColor32Array = ^TColor32Array;
  //TColor32Array = array [0..0] of TColor32;
  TColor32Array = array [0.. MaxInt div SizeOf(TColor32) -1 ] of TColor32;
  TArrayOfColor32 = array of TColor32;

 

[Anders Melander 1783]

1 hour ago, Kas Ob. said:

The best is to fix GR32 with the following, but don't do that on your own, this is up to Anders

  PColor32Array = ^TColor32Array;
  //TColor32Array = array [0..0] of TColor32;
  TColor32Array = array [0.. MaxInt div SizeOf(TColor32) -1 ] of TColor32;
  TArrayOfColor32 = array of TColor32;

 

I started to make this change for all the publicly declared array[0..0] types (there's more than a few) but then I realized that this will probably break existing code.

For example if a user of Graphics32 has the following declaration:

type
  TMyStuff = record
    FirstPixel: TColor32;
    TheRestOfThePixels: TColor32Array;
  end;
  
  PMyStuff = ^TMyStuff;

Then the compiler will refuse to compile it because the size of the structure exceeds the max allowed:

Quote

E2100 Data type too large: exceeds 2 GB

 

Given that the [0..0] declaration is a very common technique and the problem here really is that range checking has been enabled for a corpus of code that wasn't written with that in mind (and I'm speaking of *Lemmix here) I think I'll leave it be.

 

My recommendation to get around this for now is to search for PColor32Array and then surround the code with {$RANGECHECKS OFF}...{$RANGECHECKS ON} - or simply declare a new type and cast to it as @Kas Ob. has suggested. Give it a neutral name like TPixelArray/PPixelArray or something like that though; It's not really a hack.

[Kas Ob. 121]

40 minutes ago, Anders Melander said:

I realized that this will probably break existing code.

 

41 minutes ago, Anders Melander said:

Then the compiler will refuse to compile it because the size of the structure exceeds the max allowed:

This exactly the point, this type should not be used ! (no variable or field should be declared as such)

I searched GR32 source and it is in fact not used anywhere, because it should not appear anywhere, such static array can be used just to make the virtual declaration of it (pointer to) available.

 

Think about it, your example TMyStuff and even in many Windows SDK structures need such type, the easy way is to declare it as [0..0] to cast the header of record, and this exactly what make these structures not memory safe, alas there is no workaround it, though the only approach is to have two versions, or may be in case of such record then prevent the declaration of xxArr version and declare your own with "[0..0] of yyyy", this will prevent the developer from confusing these static arrays, ensuring best practice.

 

In other words, both these declaration 

  TColor32Array = array [0..0] of TColor32;
  TColor32Array = array [0.. MaxInt div SizeOf(TColor32) -1 ] of TColor32;

Can be dangerous, yet the second can't be used !, makes it safer and force you to declare your own need in place, while the first one has zero help from the compiler in protecting memory overflow at runtime, both are tuples, both have their lengths known before hand by the compiler and no runtime protection at all, one is not allowed the other is can be falling knife.

 

I am sounding is my opinion, and that is it.

 

 

ps : Wish if Pascal syntax had a modifier to force or at least warn with some type when used like 

  TColor32Array = array [0..0] of TColor32; varprohibited;     // can't be used in as field or var, yet allowed to be used in other types declaration like below.
  TColor32Array = array [0.. MaxInt div SizeOf(TColor32) -1 ] of TColor32; prohibited; // same as above
  PColor32Array = ^TColor32Array; // this is allowed 
  TUnsafeColArr = TColor32Array;  // this is allowed

 

[Kas Ob. 121]

Also i missed to point this thought about this, missed writing it in the above post while writing slowly and fixing gramer 

type
  TColor32Array = array [0..0] of TColor32;
  
  TMyStuff = record
    FirstPixel: TColor32;
    TheRestOfThePixels: TColor32Array;
  end;
  
  PMyStuff = ^TMyStuff;

This doesn't save any time, shorten the code or makes it clearer (if clearer is a word), for simple fact :

the length of data in that array is 1 for the compiler and any manipulation with such declared var (rec) will only copy one element "like assigning with := ", and an extra var/field contains the actual length should be saved somewhere, on top of that, there must be a manually code written to perform the actual copy of the elements, so... !! again my opinion.

[Anders Melander 1783]

I think you missed my point: Backward compatibility.

 

While we can agree on the pitfalls of the current declarations and on how the types could have been declared, I have to take the users' existing code into account before I change something that have worked for over 20 years.

 

3 hours ago, Kas Ob. said:

This doesn't save any time, shorten the code or makes it clearer (if clearer is a word)

Again you missed the point. It was just an example of how changing a publicly declared array type can break user code.

[Willicious 8]

On 5/31/2024 at 12:10 PM, Anders Melander said:

Excuse me but why the hell would you ask ChatGPT about that? It knows nothing about coding; It's a linguistic AI.

Yeah, it's not always the best resource for programming, but it can sometimes help with quick solutions (e.g. syntax errors) and can actually help with what you're suggesting (i.e. breaking a block of code down to analyze what it does). It absolutely still requires basic knowledge to be able to make good use of its output though.

 

I'm not a complete beginner by now, and I do understand way more about how things work than I did this time last year (for example, I know the difference between functions, methods, variables, constants, types, classes, and what these are all for). I made my first enum type a couple of weeks ago, for example, and it was satisfying to get it right and have it optimise the code I was working on. That was done with the guidance of another programmer as well. I've been very lucky to have lots of help, and I hope I can one day provide the same for someone else.

 

On 5/31/2024 at 12:10 PM, Anders Melander said:

First you need to understand what the error means (what is a range check error), then you must understand what the code does. Pick the code apart and comment it along the way. Run it in the debugger, examine the values passed to the method, validate that they are within the expected bounds, etc. etc. Continue until you completely understand what is going on.

I generally do take this approach. It's more difficult with some things than others.

 

For range check errors, from what I understand it's if something falls outside the specified bounds, for example:

 

(i = 1 to 10)

 

If I then somehow = 11, that would cause a range check error...?

Edited June 1 by Willicious

[Willicious 8]

On 5/31/2024 at 11:45 AM, msohn said:

It already does this ... if it already loaded the info, skipped the loading, i.e. a cache

From what I can tell, none of the loading is done from the level select treeview. It's all done in LemLevel and LemNeoLevelPack and then the parsed data is merely accessed from LevelSelect. But, if that's the case, then what's causing the slow response times? It's very difficult to narrow it down to any one particular thing without guidance, hence this thread.

 

EDIT: It's possible that, by accessing the data from LevelSelect, that causes it to be parsed in LemLevel. I'm not sure which way around it is.

Edited June 1 by Willicious

[msohn 28]

6 hours ago, Willicious said:

From what I can tell, none of the loading is done from the level select treeview. It's all done in LemLevel and LemNeoLevelPack and then the parsed data is merely accessed from LevelSelect.

Absolutely correct.

Quote

But, if that's the case, then what's causing the slow response times? It's very difficult to narrow it down to any one particular thing without guidance, hence this thread.

To see where the time is spent, look at the VTune screenshot I posted here a few day ago:

You have to read it from the bottom up. The x axis is time spent and it's only showing the time spent in ...BaseMenuScreen.DoLevelSelect, i.e. clicking the button to show the dialog.

 

There's basically 3 main "pillars". The right-most shows all the way from TCustomForm.Show, via your TFLevelSelect.FormShow event handler calling SetInfo. The LoadNodeLabels part is what I was referring to in my last post where only visible, uninitialised nodes are updated. That is something that I would recommend joining with .InitializeTreeView - but do it for all nodes - and show a progress while you do it.

 

The caching happens in LemLevelNeoPack, for example in TNeoLevelEntry.GetTitle. All these property getters call LoadLevelFileData, which checks LoadState telling it what was already loaded and then only load the missing parts or do nothing.

 

Finally, to be clear: someone (you?) already put quite some effort into optimising the LevelSelect dialog, i.e. to only spend as little time as possible with filling the GUI initially (think InitializeTreeView) and then load all other parts upon user-interaction (SetInfo calls). It just turned out to be too slow (maybe too many levels/groups/packs now) and a mess (calling SetInfo at too many places). Now the task is to undo all of that, do the slower loading of everything while showing a progress and giving your applications window message pump more time to "breathe" and fix the freezing. The result hopefully is a better, more responsive compromise: opening the dialog the first time will take a lot longer than now - the progress will move slowly - but subsequently, a lot quicker.

 

[Anders Melander 1783]

On 6/1/2024 at 9:41 PM, Willicious said:

For range check errors, from what I understand it's if something falls outside the specified bounds, for example:

(i = 1 to 10)

If I then somehow = 11, that would cause a range check error...?

Let me look it up in the help for you...

 

Quote

Range checking

In the {$R+} state, all array and string-indexing expressions are verified as being within the defined bounds, and all assignments to enumerated and subrange variables are checked to be within range.

Quote

Overflow checking

In the {$Q+} state, certain integer arithmetic operations (+, -, *, Abs, Sqr, Succ, Pred, Inc, and Dec) are checked for overflow. The code for each of these integer arithmetic operations is followed by additional code that verifies that the result is within the supported range.

 

[Willicious 8]

On 5/31/2024 at 11:45 AM, msohn said:

So my idea would be to fully update the tree including all the loading in InitializeTreeView, i.e. you would merge SetInfo (edit: only the part that updates the tree) and InitializeTreeView and show a progress while you do that. That would also allow to to get rid of the OnExpanded node handler you added earlier in the thread here.

 

On 6/2/2024 at 4:22 AM, msohn said:

The LoadNodeLabels part is what I was referring to in my last post where only visible, uninitialised nodes are updated. That is something that I would recommend joining with .InitializeTreeView - but do it for all nodes - and show a progress while you do it.

OK... I went ahead and:

 

1) Merged LoadIcons and LoadNodeLabels into InitializeTreeview

2) Removed the call to LoadNodeLabels in SetInfo

3) Moved the call to LoadIcons to DisplayLevelInfo (still called via SetInfo, but at least it's now in the right place!)

4) Removed the OnExpanded event handler

 

And WOW! The difference is night and day. It takes much longer to load the Level Select initially (expected, and I'll do a progress bar for that), but once open it's super snappy and responsive, and doesn't bug out when switching between packs any more 🎉

 

Thanks for your help in narrowing down what was causing the delays. I clearly need to install VTune (is there a free/budget version?) as that displays exactly what's causing the delays very clearly, so it will no doubt come in very handy for other features of the program.

 

The only thing I'd probably say is that it might be somewhat frustrating to use the slower-loading Level Select when only wanting to switch quickly between levels (e.g. for testing purposes). It might be worth doing a "Quick Select" version which only displays the treeview and nothing else; this would likely be the default. Then, a button for "Show Level Info" could go ahead and load the full version. I'll see what the feedback is after releasing this slower-loading but quicker-interfacing version of the Level Select.

 

I'd say that pretty much wraps up this topic as resolved, but it would be good to continue the discussion around general optimization. Can the admins split the posts regarding range & overflow checking into a new topic? Happy to continue that discussion here if not.

 

----------------------

 

EDIT: After playing around with it for a bit, I think the (much!) slower loading time is too much of a cost for the slightly snappier treeview. Clicking into Level Select is done quite a lot, especially for testing purposes, and it gets old quick; I can see why it was set up the way it is (i.e. loading node labels only when actually needed). There may be other opportunities for optimization though; I'll take a look at the VTune diagram and see what's possible.

 

Also, I've learned a ton via this thread, so thank you again!

Edited June 4 by Willicious

[msohn 28]

I'm glad I took the time to explain how I read the VTune flame graph and that you tried and verified most of what I expected (well guessed tbh). And that you now saw first-hand why the original code was written the way it was. Welcome to the world of maintaining legacy projects😉

 

I'm sure you figured out how to get a copy of VTune, if not refer to the thread by Anders which I linked to in the flame graph post. It contained all I needed to get going.

 

I read your post like you reverted your changes for now, but you might want to consider a few things which could improve the solution you tried:

 

1. Make sure all the loaded info is kept, i.e. when opening the dialog a second time, everything is already there. That way the user has to endure the delay only once. If that proves that memory usage is of no real concern (which I'd expect), you could go ahead and

2. Consider loading the info asynchronously, i.e. in a thread/threadpool as soon as the app starts - then, if the users opens the dialog before the loading has finished, show a progress and wait for it to finish. This should behave great when it works, but making that codebase thread safe is an enormous task, especially since there don't seem to be unit- and functional tests which help you make changes with confidence 

[Willicious 8]

On 6/4/2024 at 1:58 PM, msohn said:

1. Make sure all the loaded info is kept, i.e. when opening the dialog a second time, everything is already there. That way the user has to endure the delay only once

I'm not sure exactly how to do this. It was my (and, from what you've been saying, your) understanding that the program already did cache the loaded info, but it turns out it doesn't (at least not to any noticeable extent). I think it has to reload everything even in the same session (i.e. with SLX still open, but Level Select being closed and then re-opened).

[Willicious 8]

On 5/27/2024 at 3:56 PM, msohn said:

Not with the built-in tools, but here's an excellent summary by Remy of what you need to do: https://stackoverflow.com/a/1130506/386473

 

I just used exactly that approach and was able to find and fix the memory leaks in like half an hour. I'll try to make a pull request on GitHub as the patch is a bit larger due to spacing when I introduced new try/finally blocks.

Got one more memory leak that's showed up and I'd like to have a go at fixing it myself. It's narrowed down to being a problem with TProjectile, which doesn't have a destructor. However, I implemented a destructor and this doesn't fix the memory leak, so... more investigation needed.

 

I'm having some problems getting FastMM set up. Managed all but one step:

 

I tried running the program anyway and as far as I can tell, nothing happens. Does it open a dialog or spit out a text file? Or something else?

[JonRobertson 72]

4 hours ago, Willicious said:

It's narrowed down to being a problem with TProjectile, which doesn't have a destructor. However, I implemented a destructor and this doesn't fix the memory leak

Classes do not need a destructor to be destroyed/freed. A class needs a destructor if it allocated other objects or memory that it should free or there is other cleanup it needs to do before being destroyed.

 

If an object is not being freed, adding a destructor would not fix the issue.

4 hours ago, Willicious said:

Does it open a dialog or spit out a text file? Or something else?

Read the comments in FastMM4Options.inc. Specifically:

  {Set this option to log all errors to a text file in the same folder as the
   application. Memory errors (with the FullDebugMode option set) will be
   appended to the log file. Has no effect if "FullDebugMode" is not set.}
  {$define LogErrorsToFile}

  {Set this option to log all memory leaks to a text file in the same folder as
   the application. Memory leak reports (with the FullDebugMode option set)
   will be appended to the log file. Has no effect if "LogErrorsToFile" and
   "FullDebugMode" are not also set. Note that usually all leaks are always
   logged, even if they are "expected" leaks registered through
   AddExpectedMemoryLeaks. Expected leaks registered by pointer may be excluded
   through the HideExpectedLeaksRegisteredByPointer option.}
  {$define LogMemoryLeakDetailToFile}

It has been a while since I did this. I found a post on SO for a reminder. Based on that post, copy FastMM4Options.inc to your project folder. Edit the .inc file and remove the period from {.$define FullDebugMode}

 

Do a full build of the project and any leaks should be written to a text file with additional information about the object leaked and when it was allocated.

Edited June 20 by JonRobertson

[Anders Melander 1783]

10 hours ago, Willicious said:

I'm having some problems getting FastMM set up.

Give madExcept a try again; It's much easier.

[Willicious 8]

@Anders Melander

Thanks for the advice, I installed madExcept again and it generated the following leak report:

 

The problem was in TRenderer.DrawProjectileShadow (shown in the call stack), in which an instance of TProjectile was being created and not freed. I've now fixed this.

 

madExcept is super useful then, no doubt. I did however uninstall it again after use because I noticed significant lag when running SLX with it enabled, and it also caused some access violation error dialogs to popup. I wonder whether this may be because I installed a newer version of madExcept which is intended to work with Delphi 12 and I'm still on Delphi 10.4 - It might be worth trying a previous version next time; the dev leaves all versions up on the site, helpfully.

[Willicious 8]

On 6/20/2024 at 2:48 AM, JonRobertson said:

copy FastMM4Options.inc to your project folder. Edit the .inc file and remove the period from {.$define FullDebugMode}

 

Do a full build of the project and any leaks should be written to a text file with additional information about the object leaked and when it was allocated.

Thanks for this. I'll give it a try - should come in handy for future memory leaks. Can it also be used for overflow/range checking as well?

Edited June 28 by Willicious

[Anders Melander 1783]

7 hours ago, Willicious said:

madExcept is super useful then, no doubt. I did however uninstall it again after use because I noticed significant lag when running SLX with it enabled

You don't need to uninstall it; Just disable it for the project (i.e. uncheck the "enable madExcept" checkbox). It doesn't do anything and there's no overhead when it isn't enabled for the project.

 

A significant overhead is expected, both in terms of performance and memory usage, when resource tracking is enabled and it's even worse when buffer overrun detection is enabled. So you should only enable these two features during debug, either to track down a suspected leak/overwrite or to occasionally verify that there aren't any once you get there.

The core functionality, which is exception handling/stack tracing, doesn't cause any performance overhead.

 

7 hours ago, Willicious said:

it also caused some access violation error dialogs to popup.

Those are almost certainly errors in your code.

madExcept should produce a call stack that points to the origin of the exception. If it doesn't (i.e. the exception is only visible in the debugger) then it's because the code is actively swallowing the exception. A practice that unfortunately is quite common in legacy code:

try
  ...lots and lots of code here...
except
  // I can't figure out why the code above fails occasionally
  // so let's just pretend there's no problem :-/
end;

 

 

7 hours ago, Willicious said:

I wonder whether this may be because I installed a newer version of madExcept which is intended to work with Delphi 12 and I'm still on Delphi 10.4

No. New version are backward compatible; You don't need to install an older version.

[Willicious 8]

Struggling to make sense of this one. I've found a resource which isn't being freed ("NewAnim", a TGadgetAnimation instance), but all attempts to free it result in either the program not running at all, or the resource leak persisting anyway:

 

procedure TGadgetAnimation.Load(aCollection, aPiece: String; aSegment: TParserSection; aTheme: TNeoTheme);
var
  BaseTrigger: TGadgetAnimationTrigger;
  LoadPath: String;
  S: String;

  NeedUpscale: Boolean;
  Bitmaps: TBitmaps;
  i: Integer;

  Info: TUpscaleInfo;
begin
  Clear;

  fFrameCount := aSegment.LineNumeric['frames'];
  fName := UpperCase(aSegment.LineTrimString['name']);
  fColor := UpperCase(aSegment.LineTrimString['color']);

  if LeftStr(fName, 1) <> '*' then
  begin
    if GameParams.HighResolution then
      LoadPath := AppPath + SFStyles + aCollection + SFPiecesObjectsHighRes + aPiece
    else
      LoadPath := AppPath + SFStyles + aCollection + SFPiecesObjects + aPiece;

    if fName <> '' then
      LoadPath := LoadPath + '_' + fName; // For backwards-compatible or simply unnamed primaries
    LoadPath := LoadPath + '.png';

    if GameParams.HighResolution and not FileExists(LoadPath) then
    begin
      LoadPath := AppPath + SFStyles + aCollection + SFPiecesObjects + aPiece;

      if fName <> '' then
        LoadPath := LoadPath + '_' + fName; // For backwards-compatible or simply unnamed primaries
      LoadPath := LoadPath + '.png';

      NeedUpscale := true;
    end else
      NeedUpscale := false;

    fHorizontalStrip := aSegment.Line['horizontal_strip'] <> nil;

    TPngInterface.LoadPngFile(LoadPath, fSourceImage);

    if fHorizontalStrip then
    begin
      fWidth := fSourceImage.Width div fFrameCount;
      fHeight := fSourceImage.Height;
    end else begin
      fWidth := fSourceImage.Width;
      fHeight := fSourceImage.Height div fFrameCount;
    end;

    if NeedUpscale then
    begin
      Bitmaps := MakeFrameBitmaps(true);
      Info := PieceManager.GetUpscaleInfo(aCollection + ':' + aPiece, rkGadget);
      for i := 0 to Bitmaps.Count-1 do
        Upscale(Bitmaps[i], Info.Settings);
      CombineBitmaps(Bitmaps);
    end else if GameParams.HighResolution then
    begin
      fWidth := fWidth div 2;
      fHeight := fHeight div 2;
    end;
  end else begin
    fHorizontalStrip := false;

    if Lowercase(fName) = '*blank' then
    begin
      fWidth := aSegment.LineNumeric['WIDTH'];
      fHeight := aSegment.LineNumeric['HEIGHT'];
      // Preserve previously-loaded frame count.

      fSourceImage.SetSize(fWidth * ResMod, fHeight * ResMod * fFrameCount);
      fSourceImage.Clear(0);
    end else begin
      // Fallback behaviour. Could mean it's unrecognized, or handled elsewhere (eg. "*PICKUP").
      fSourceImage.SetSize(ResMod, ResMod);
      fSourceImage.Clear(0);
      fFrameCount := 1;
      fWidth := 1;
      fHeight := 1;
    end;
  end;

  // Only set fPrimary by TGadgetAnimations
  if fPrimary and (aSegment.Line['z_index'] = nil) then
    fZIndex := 1
  else
    fZIndex := aSegment.LineNumeric['z_index'];

  if Uppercase(aSegment.LineTrimString['initial_frame']) = 'RANDOM' then
    fStartFrameIndex := -1
  else
    fStartFrameIndex := aSegment.LineNumeric['initial_frame'];

  if fPrimary then
  begin
    fMainObjectWidth := fWidth;
    fMainObjectHeight := fHeight;
  end;

  fOffsetX := aSegment.LineNumeric['offset_x'];
  fOffsetY := aSegment.LineNumeric['offset_y'];

  fCutTop := aSegment.LineNumeric['nine_slice_top'];
  fCutRight := aSegment.LineNumeric['nine_slice_right'];
  fCutBottom := aSegment.LineNumeric['nine_slice_bottom'];
  fCutLeft := aSegment.LineNumeric['nine_slice_left'];

  BaseTrigger := TGadgetAnimationTrigger.Create;

  S := Lowercase(aSegment.LineTrimString['state']);

  if (S = 'pause') then
    BaseTrigger.fState := gasPause
  else if (S = 'stop') then
    BaseTrigger.fState := gasStop
  else if (S = 'looptozero') then
    BaseTrigger.fState := gasLoopToZero
  else if (S = 'matchphysics') then
    BaseTrigger.fState := gasMatchPrimary
  else if (aSegment.Line['hide'] <> nil) then
    BaseTrigger.fState := gasPause
  else
    BaseTrigger.fState := gasPlay;

  if (aSegment.Line['hide'] = nil) then
    BaseTrigger.fVisible := true
  else
    BaseTrigger.fVisible := false;

  fTriggers.Add(BaseTrigger);

  if fPrimary then
  begin
    // Some properties are overridden/hardcoded for primary
    BaseTrigger.fState := gasPause; // Physics control the current frame
    BaseTrigger.fVisible := true;   // Never hide the primary - if it's needed as an effect, make the graphic blank
  end else begin
    // If NOT primary - load triggers
    aSegment.DoForEachSection('trigger',
      procedure(aSec: TParserSection; const aCount: Integer)
      var
        NewTrigger: TGadgetAnimationTrigger;
      begin
        NewTrigger := TGadgetAnimationTrigger.Create;
        NewTrigger.Load(aSec);
        fTriggers.Add(NewTrigger);
      end
    );
  end;

  fNeedRemask := true;
  fMaskColor := $FFFFFFFF;
end;

procedure TGadgetMetaInfo.Load(aCollection,aPiece: String; aTheme: TNeoTheme);
var
  Parser: TParser;
  Sec: TParserSection;

  GadgetAccessor: TGadgetMetaAccessor;
  NewAnim: TGadgetAnimation;
  PrimaryWidth: Integer;
begin
  fGS := Lowercase(aCollection);
  fPiece := Lowercase(aPiece);
  GadgetAccessor := GetInterface(false, false, false);

  Parser := TParser.Create;
  try
    ClearImages;

    if not DirectoryExists(AppPath + SFStyles + aCollection + SFPiecesObjects) then
      raise Exception.Create('TMetaObject.Load: Collection "' + aCollection + '" does not exist or does not have objects. (' + aPiece + ')');
    SetCurrentDir(AppPath + SFStyles + aCollection + SFPiecesObjects);

    Parser.LoadFromFile(aPiece + '.nxmo');
    Sec := Parser.MainSection;

    // Trigger effects
    if Lowercase(Sec.LineTrimString['effect']) = 'exit' then fTriggerEffect := DOM_EXIT;
    if Lowercase(Sec.LineTrimString['effect']) = 'forceleft' then fTriggerEffect := DOM_FORCELEFT;
    if Lowercase(Sec.LineTrimString['effect']) = 'forceright' then fTriggerEffect := DOM_FORCERIGHT;
    if Lowercase(Sec.LineTrimString['effect']) = 'trap' then fTriggerEffect := DOM_TRAP;
    if Lowercase(Sec.LineTrimString['effect']) = 'water' then fTriggerEffect := DOM_WATER;
    if Lowercase(Sec.LineTrimString['effect']) = 'fire' then fTriggerEffect := DOM_FIRE;
    if Lowercase(Sec.LineTrimString['effect']) = 'onewayleft' then fTriggerEffect := DOM_ONEWAYLEFT;
    if Lowercase(Sec.LineTrimString['effect']) = 'onewayright' then fTriggerEffect := DOM_ONEWAYRIGHT;
    if Lowercase(Sec.LineTrimString['effect']) = 'teleporter' then fTriggerEffect := DOM_TELEPORT;
    if Lowercase(Sec.LineTrimString['effect']) = 'receiver' then fTriggerEffect := DOM_RECEIVER;
    if Lowercase(Sec.LineTrimString['effect']) = 'pickupskill' then fTriggerEffect := DOM_PICKUP;
    if Lowercase(Sec.LineTrimString['effect']) = 'lockedexit' then fTriggerEffect := DOM_LOCKEXIT;
    if Lowercase(Sec.LineTrimString['effect']) = 'unlockbutton' then fTriggerEffect := DOM_BUTTON;
    if Lowercase(Sec.LineTrimString['effect']) = 'collectible' then fTriggerEffect := DOM_COLLECTIBLE;
    if Lowercase(Sec.LineTrimString['effect']) = 'onewaydown' then fTriggerEffect := DOM_ONEWAYDOWN;
    if Lowercase(Sec.LineTrimString['effect']) = 'updraft' then fTriggerEffect := DOM_UPDRAFT;
    if Lowercase(Sec.LineTrimString['effect']) = 'splitter' then fTriggerEffect := DOM_SPLITTER;
    if Lowercase(Sec.LineTrimString['effect']) = 'entrance' then fTriggerEffect := DOM_WINDOW;
    if Lowercase(Sec.LineTrimString['effect']) = 'antisplatpad' then fTriggerEffect := DOM_NOSPLAT;
    if Lowercase(Sec.LineTrimString['effect']) = 'splatpad' then fTriggerEffect := DOM_SPLAT;
    if Lowercase(Sec.LineTrimString['effect']) = 'decoration' then fTriggerEffect := DOM_DECORATION;
    if Lowercase(Sec.LineTrimString['effect']) = 'traponce' then fTriggerEffect := DOM_TRAPONCE;
    if Lowercase(Sec.LineTrimString['effect']) = 'onewayup' then fTriggerEffect := DOM_ONEWAYUP;
    if Lowercase(Sec.LineTrimString['effect']) = 'animation' then fTriggerEffect := DOM_ANIMATION;
    if Lowercase(Sec.LineTrimString['effect']) = 'animationonce' then fTriggerEffect := DOM_ANIMONCE;
    if Lowercase(Sec.LineTrimString['effect']) = 'blasticine' then fTriggerEffect := DOM_BLASTICINE;
    if Lowercase(Sec.LineTrimString['effect']) = 'vinewater' then fTriggerEffect := DOM_VINEWATER;
    if Lowercase(Sec.LineTrimString['effect']) = 'poison' then fTriggerEffect := DOM_POISON;
    if Lowercase(Sec.LineTrimString['effect']) = 'lava' then fTriggerEffect := DOM_LAVA;
    if Lowercase(Sec.LineTrimString['effect']) = 'radiation' then fTriggerEffect := DOM_RADIATION;
    if Lowercase(Sec.LineTrimString['effect']) = 'slowfreeze' then fTriggerEffect := DOM_SLOWFREEZE;

    if (Lowercase(Sec.LineTrimString['effect']) = 'decoration')
    or (Lowercase(Sec.LineTrimString['effect']) = 'paint') then
      fTriggerEffect := DOM_DECORATION;

    if Sec.Section['PRIMARY_ANIMATION'] = nil then
    begin
      if LastWarningStyle <> fGS then
      begin
        ShowMessage('Gadget ' + fGS + ':' + fPiece + ' is in pre-12.7 format. Please update your copy of this style, or if up to date, ask the style creator to fix.');
        LastWarningStyle := fGS;
      end;
      raise Exception.Create('Gadget ' + fGS + ':' + fPiece + ' is in pre-12.7 format. Please update your copy of this style, or if up to date, ask the style creator to fix.');
    end;

    NewAnim := TGadgetAnimation.Create(0, 0);
    GadgetAccessor.Animations.AddPrimary(NewAnim);
    NewAnim.Load(aCollection, aPiece, Sec.Section['PRIMARY_ANIMATION'], aTheme);

    fFrameCount := NewAnim.FrameCount;
    PrimaryWidth := NewAnim.Width; // Used later

    Sec.DoForEachSection('ANIMATION',
      procedure (aSection: TParserSection; const aIteration: Integer)
      begin
        NewAnim := TGadgetAnimation.Create(GadgetAccessor.Animations.PrimaryAnimation.Width, GadgetAccessor.Animations.PrimaryAnimation.Height);
        GadgetAccessor.Animations.Add(NewAnim);
        NewAnim.Load(aCollection, aPiece, aSection, aTheme);
      end
    );

    GadgetAccessor.Animations.SortByZIndex;

    GadgetAccessor.TriggerLeft := Sec.LineNumeric['trigger_x'];
    GadgetAccessor.TriggerTop := Sec.LineNumeric['trigger_y'];
    GadgetAccessor.TriggerWidth := Sec.LineNumeric['trigger_width'];
    GadgetAccessor.TriggerHeight := Sec.LineNumeric['trigger_height'];

    GadgetAccessor.DefaultWidth := Sec.LineNumeric['default_width'];
    GadgetAccessor.DefaultHeight := Sec.LineNumeric['default_height'];

    GadgetAccessor.DigitX := Sec.LineNumericDefault['digit_x', PrimaryWidth div 2];
    GadgetAccessor.DigitY := Sec.LineNumericDefault['digit_y', -6];

    GadgetAccessor.ExitMarkerX := Sec.LineNumeric['exit_marker_x'];
    GadgetAccessor.ExitMarkerY := Sec.LineNumeric['exit_marker_y'];

    if LeftStr(Lowercase(Sec.LineTrimString['digit_alignment']), 1) = 'l' then
      GadgetAccessor.DigitAlign := -1
    else if LeftStr(Lowercase(Sec.LineTrimString['digit_alignment']), 1) = 'r' then
      GadgetAccessor.DigitAlign := 1
    else
      GadgetAccessor.DigitAlign := 0;

    fDigitMinLength := Sec.LineNumericDefault['digit_length', 1];

    if Sec.Line['sound_activate'] = nil then
      fSoundActivate := Sec.LineTrimString['sound']
    else
      fSoundActivate := Sec.LineTrimString['sound_activate'];
    fSoundExhaust := Sec.LineTrimString['sound_exhaust'];

    fKeyFrame := Sec.LineNumeric['key_frame']; // This is almost purely a physics property, so should not go under animations

    if Sec.Line['resize_both'] <> nil then
    begin
      GadgetAccessor.CanResizeHorizontal := true;
      GadgetAccessor.CanResizeVertical := true;
    end else begin
      GadgetAccessor.CanResizeHorizontal := Sec.Line['resize_horizontal'] <> nil;
      GadgetAccessor.CanResizeVertical := Sec.Line['resize_vertical'] <> nil;
    end;

    if fTriggerEffect in [DOM_NONE, DOM_DECORATION] then // No trigger area
    begin
      GadgetAccessor.TriggerWidth := 0;
      GadgetAccessor.TriggerHeight := 0;
    end;

    if fTriggerEffect in [DOM_RECEIVER, DOM_WINDOW] then // Trigger point only
    begin
      GadgetAccessor.TriggerWidth := 1;
      GadgetAccessor.TriggerHeight := 1;
    end;
  finally
    Parser.Free;
  end;
end;

 

[Willicious 8]

Here's the regular memory leak dialog:

 

[Anders Melander 1783]

My guess is that it's the TGadgetAnimation.fName string which is leaked (based on the string content and the call to UpperCase) but that it a bit strange because strings are reference counted and it's usually pretty impossible to leak them.

It's also strange that madExcept only reports the string leaked and not the object(s) that references the string. I don't think I've seen that before.

 

I think you maybe have several bugs in play which messes with the resource tracking. If possible, try to build and run with "Instantly crash on buffer (*) overrun" enabled.

I would suspect that there are problems with objects being referenced after they have been destroyed. One easy thing you can do to catch some of those cases is to use FreeAndNil instead of Free when destroying objects. Don't bother doing it on local vars (i.e. vars declared in a function/method). Do it on member vars (i.e. declared in a class).

[Willicious 8]

2 hours ago, Anders Melander said:

If possible, try to build and run with "Instantly crash on buffer (*) overrun" enabled.

Thanks, I've given this a try. It crashed on attempting to load a level with Pickup skill objects. Here's the call stack:

 

 

Does this give us any more clues as to what to target? Should I FreeAndNil everything in TGadgetAnimations just to be sure?

[Anders Melander 1783]

1 hour ago, Willicious said:

It crashed on attempting to load a level with Pickup skill objects. Here's the call stack:

So you are reading or writing from/to memory that has been free'd. That's a bug.

The call stack tells you the unit and exact line number (if you ran it in the debugger you would get the same information) and it should be easy to see what the problem is.

 

1 hour ago, Willicious said:

Does this give us any more clues as to what to target?

Start by fixing the bugs surfaced by the buffer overrun detection. Once they are fixed you can focus on the leaks. The leaks are probably insignificant. The other bugs are definitely not.

 

With regard to FreeAndNil you need to understand what it does and what the purpose of using it is (in this case, not generally) and then it should become self-evident where to use it. But do that later.

[Kas Ob. 121]

10 hours ago, Willicious said:

That is 58 object leak of TGadgetMetaAccesseor !

This should be easy to track and fix, fixing this part most likely will fix most of the others in that screenshot.

 

Now, and from the pasted code, i can't figure out where "GadgetAccessor: TGadgetMetaAccessor;" is freed, these are local variables so where are they used and to whom they are shipped, notice and follow how these variables are referencing each other.

procedure TGadgetMetaInfo.Load(aCollection,aPiece: String; aTheme: TNeoTheme);
var
..
  GadgetAccessor: TGadgetMetaAccessor;
  NewAnim: TGadgetAnimation;
..
begin
..
  GadgetAccessor := GetInterface(false, false, false);  // <-----
..

    NewAnim := TGadgetAnimation.Create(0, 0);           // <-----
    GadgetAccessor.Animations.AddPrimary(NewAnim);
    NewAnim.Load(aCollection, aPiece, Sec.Section['PRIMARY_ANIMATION'], aTheme);

    fFrameCount := NewAnim.FrameCount;
    PrimaryWidth := NewAnim.Width; // Used later

    Sec.DoForEachSection('ANIMATION',
      procedure (aSection: TParserSection; const aIteration: Integer)
      begin
        NewAnim := TGadgetAnimation.Create(GadgetAccessor.Animations.PrimaryAnimation.Width, GadgetAccessor.Animations.PrimaryAnimation.Height);
        GadgetAccessor.Animations.Add(NewAnim);
        NewAnim.Load(aCollection, aPiece, aSection, aTheme);
      end
    );
...
end;

Where and how these are released, that what you should find and fix, as for the UnicodeString leak, it might be simply a confusion/confliction in madshi with MemoryManager, that lead to miss identifying the leaks and their types.

[Willicious 8]

9 hours ago, Anders Melander said:

The call stack tells you the unit and exact line number (if you ran it in the debugger you would get the same information) and it should be easy to see what the problem is.

 

Start by fixing the bugs surfaced by the buffer overrun detection.

Here's the last 2 items in the call stack (I assume that only the most recent lines in the call stack are relevant?):

procedure TGadgetAnimations.AddPrimary(aAnimation: TGadgetAnimation);
begin
  Add(aAnimation);
  if fPrimaryAnimation <> nil then
    fPrimaryAnimation.fPrimary := false; <---------------------------------------- this is line 935
  fPrimaryAnimation := aAnimation;
  aAnimation.fPrimary := true;
end;

procedure TGadgetAnimations.Clone(aSrc: TGadgetAnimations);
var
  i: Integer;
  NewAnim: TGadgetAnimation;
begin
  Clear;

  NewAnim := TGadgetAnimation.Create(aSrc.PrimaryAnimation.fMainObjectWidth, aSrc.PrimaryAnimation.fMainObjectHeight);
  NewAnim.Clone(aSrc.PrimaryAnimation);
  AddPrimary(NewAnim); <------------------------------------------------- this is line 949

  for i := 0 to aSrc.Count-1 do
  begin
    if aSrc.Items[i].Primary then
      Continue;

    NewAnim := TGadgetAnimation.Create(aSrc.Items[i].fMainObjectWidth, aSrc.Items[i].fMainObjectHeight);
    NewAnim.Clone(aSrc.Items[i]);

    Add(NewAnim);
  end;

  SortByZIndex;
end;

This points to fPrimaryAnimation being the problem. Maybe this has already been freed and then not re-created for use here (best guess from what you've said)?

 

Question is, which is the best action to take:

 

1) Free fPrimaryAnimation later

2) Re-create and re-free fPrimaryAnimation

3) Something else?