Fix demand-loaded building artwork ownership

[Krisztiaan]

Summary

Fix demand-loaded building artwork ownership and deallocation, preventing heap corruption in mods such as TFADW.

Behavior and compatibility

Bug fix for DemandLoad, DemandLoadBuildup, and FreeBuildup. No format, save, replay, networking, determinism, COM, or ABI changes.

Validation

• VS2022 Win32 Debug and Release builds and tests passed.
• Exact Release artifact tested under Wine with TFADW 1.02 and UnitCount=1.
• Skirmish loaded, demand-loaded artwork initialized, and gameplay began without the original crash.
• Native Windows runtime not tested.

Documentation

Updated the affected key documentation, MIX ownership guidance, and change record.

Checklist

• The change is focused; unrelated mechanical cleanup is separate
• Compatibility effects and any migration are explicit
• Validation distinguishes what passed, failed, and was not run
• No prohibited assets, binaries, SDKs, credentials, or generated output are included

[ZivDero]

Had Claude review it:

Reviewed the ownership model this change establishes — demand-loaded art is owned by the type and freed with delete[] (char*), MFCD::Retrieve pointers are borrowed and must never be freed. That framing is right, and Free_Demand_Loaded_Shape matches Load_Alloc_Data's new char[size] (code/data.cpp:67), so the mismatched-delete half is correctly fixed.

Two paths still leave an archive-owned pointer under an owning flag, so the original crash survives the fix.

Blocking

1. Post_Load — the guard runs too late (code/builtype.cpp:1735)

if (!IsDemandLoad) Fetch_Normal_Image(); skips this class's own re-fetch, but BASECLASS::Post_Load() on the line above reaches TechnoTypeClass::Post_Load (code/techtype.cpp:845), which does ImageData = MFCD::Retrieve(fname) unconditionally for every techno type.

After loading a save, a DemandLoad=yes type whose art lives in a cached MIX therefore holds an archive pointer with IsDemandLoad restored true (Serialize, line 1873). Get_Image_Data sees non-NULL and never demand-loads, and the next teardown — RulesClass::Initialize's while (BuildingTypes.Count()) delete BuildingTypes[0]; (code/rules.cpp:651) — runs the destructor's delete[] into the archive's cache block. The theater Init frees (609/638) and this PR's own Read_INI pre-free (1134) hit the same pointer.

Skipping the re-fetch isn't enough; the pointer has to be detached:

if (IsDemandLoad) {
    ImageData = NULL;
} else {
    Fetch_Normal_Image();
}

2. No BuildupData counterpart to the ImageData detach (code/builtype.cpp:1277)

Read_INI detaches ImageData once the flags are read, but there's no matching reset for buildup. If DemandLoadBuildup flips no→yes across passes — a map INI overriding Image= to an art section that sets it, with RulesClass::Objects re-running Read_INI per INI layer (code/rules.cpp:2802) — then the top-of-function free correctly skips (old flag false), the flag flips true at 1275, and Fetch_Building_Normal_Image skips the refetch at 955. The archive pointer from the earlier pass simply stays, and is later freed as if owned by Free_Buildup_Data, theater Init, the next Read_INI, or the destructor.

Needs the symmetric if (IsDemandLoadBuildup) { BuildupData = NULL; }.

3. The change record targets a released version

release: 0.1.0, but the branch predates the 0.1.0 release and is behind main, where 0.1.0 is released and 0.2.0 is development. manual/tools/versioning.py:599 enforces "new changes must target development release", matching manual/AUTHORING.md:219. Needs retargeting to 0.2.0 on rebase.

Worth addressing

Free_Buildup_Data changes gameplay beyond what's documented (code/builtype.cpp:2062)

Leaving BuildupData non-NULL for FreeBuildup=yes / DemandLoadBuildup=no types makes BuildingClass::HasBuildupData true for every structure of the type, not just the first. Sellability is presumably the intended fix, but that member also gates if (HasBuildupData && i->Class->IsNominal) i->IsTechnician = true; (code/building.cpp:2525) — destroyed structures now spawn technicians where they previously spawned ordinary crew — and it's serialized at code/building.cpp:8811. Worth classifying explicitly per the AGENTS.md rule on externally visible behavior.

The Read_INI frees run before the section check (code/builtype.cpp:1134)

They sit above if (BASECLASS::Read_INI(ini)), so every INI layer discards the demand-loaded art of every demand-load type — including types that INI never mentions. A map INI overriding one Cost= forces a full re-read from disk. The BuildupData half prevents no leak in the stable-flag case, since line 955 already guards the refetch.

Documentation accuracy

• demandload--buildingtype.md: "held until the theater changes or the type is destroyed" is wrong twice — this PR adds a release at every Read_INI, and Init has no else branch, so a demand-load type with neither Theater= nor NewTheater= is never released on a theater change (the deleted text carried that qualifier).
• freebuildup.md: the "three moments" list omits a fourth Free_Buildup_Data call site in Get_Draw_Rect (code/builtype.cpp:1984).
• demandloadbuildup.md wasn't updated and still frames FreeBuildup alone as dangerous rather than inert, contradicting the rewritten freebuildup.md.
• mix.md's revised clause now cites the one key that no longer hands archive memory back, while demandload--animtype.md and demandload--overlaytype.md still carry live danger callouts for the hazard the sentence describes.

Follow-up (not this PR)

Only three classes carry a demand-load flag, and the other two have the same defects: AnimTypeClass::Post_Load (animtype.cpp:475) and OverlayTypeClass::Post_Load (overtype.cpp:420) both call Fetch_Normal_Image() ahead of their guards, and both read their flag in Read_INI after the base-class fetch with no detach — so they reach the archive-free on an ordinary rules load, no save game needed. overtype.cpp:136 also still uses the scalar delete (ShapeSet *) this PR replaced.

Since every base-class write of ImageData happens in just two places (objtype.cpp:619 and techtype.cpp:845), a check at the fetch would cover all three classes at once and make this whole class of bug unwritable. Related: tracking ownership per pointer rather than inferring it from a flag would also restore the zero-copy path — detaching the archive pointer means types whose art does resolve now heap-duplicate the whole SHP on every scenario load, where before they used the resident pointer for free.

All static analysis against the PR head; I didn't build or run it.