Final Smart Pointer and Crystal NPC Audit

Final Smart Pointer and Crystal NPC Audit

Status: April 28, 2026
Branch: refactor/atlas-smartptr
Target: TFS 1.8 Downgrade 8.60

This page documents the final ownership audit and the NPC compatibility layer added for Crystal Server RevScript NPC packs.

---

Executive Summary

TFS 1.8-8.60 is now fully RAII-based for normal engine ownership:

Area	Result
Common owning new T in src/	0
Common owning delete ptr in src/	0
std::make_unique / make_unique< hits	128
std::make_shared / make_shared< hits	75
unique_ptr references	130
shared_ptr references	373
weak_ptr references	84
Valgrind memcheck result	0 errors, 0 leaks

The generic grep counts for new and delete are intentionally not used as the final truth because they include false positives such as SQL DELETE FROM, deleted constructors (= delete), comments, strings, Lua method names such as :delete, placement-new, and allocator internals.

The meaningful result is:

Common owning raw new/delete in src/: 0

---

Important Distinction: Raw Pointer Syntax vs Raw Ownership

The source still contains raw pointer syntax such as Player*, Item*, Creature*, and Tile*.

That is not the same as unsafe ownership.

These pointers are used as non-owning observers, temporary function parameters, fast indexes, or compatibility handles. The owning lifetime is held by shared_ptr, unique_ptr, containers, Lua userdata, or controlled engine registries.

Examples of valid raw-looking code:

Pattern	Why it is valid
Player* player = ...	Non-owning local observer, lifetime anchored elsewhere.
Thing*, Item*, Tile* parameters	Existing TFS APIs pass observed objects without taking ownership.
Placement-new in luascript.cpp / luascript.h	Constructs objects inside memory owned by Lua userdata.
operator new/delete in lockfree.h	STL allocator contract: returns raw unconstructed storage, not object ownership.
OpenSSL *_new() calls	Wrapped immediately by unique_ptr with custom deleters.

Do not blindly convert every T* to shared_ptr<T>. That can create duplicated ownership, cycles, and unnecessary atomic ref-count overhead.

---

Current Ownership Model

The core engine now follows this model:

Engine Area	Ownership Model
Players	unordered_map<uint32_t, weak_ptr<Player>> indexes + shared lifetime anchors
Creatures	shared_ptr<Creature> lifetime, weak_ptr relationships
Monsters / NPCs	shared lifetime where needed, unique_ptr for strict local ownership
Items	shared_ptr<Item> factory model
Tiles	unique_ptr<Tile> ownership, smart pointer containers for contents
Conditions	unique_ptr<Condition>
Scheduler / dispatcher tasks	unique_ptr<Task> / unique_ptr<SchedulerTask>
Lua userdata	safe construction, ownership handoff, weak_ptr validation for creatures
NPC handlers	unique_ptr<NpcEventsHandler>

This matches the robust Atlas-style ownership direction while keeping TFS 1.8-8.60 protocol-specific behavior.

---

Definitive Valgrind Command

Use:

bash tools/run-valgrind.sh

The helper builds the Valgrind preset, copies the resulting binary to ./tfs, keeps the working directory at the repository root, and runs:

valgrind \
  --tool=memcheck \
  --leak-check=full \
  --show-leak-kinds=definite \
  --track-origins=yes \
  --num-callers=50 \
  --leak-resolution=high \
  --error-limit=no \
  --expensive-definedness-checks=yes \
  --partial-loads-ok=yes \
  --log-file=valgrind-definitive.log \
  ./tfs

Expected successful result:

All heap blocks were freed -- no leaks are possible
ERROR SUMMARY: 0 errors from 0 contexts

---

Crystal Server RevScript NPC Compatibility

The NPC loader now supports Crystal Server RevScript packs without hardcoded folder names.

Supported layout:

data/npc/
├── lib/
│   ├── npc.lua
│   └── crystalcompat/
├── lua/
│   └── original_tfs_npcs.lua
├── npc_CRYSTALSERVER_REV/
│   ├── alesar.lua
│   └── subfolder/
│       └── Delowen.lua
└── any_custom_folder_name/
    └── nested/
        └── npc.lua

Rules:

• data/npc/lib/ is treated as library code and is not scanned as NPC definitions.
• Every other direct folder inside data/npc/ is scanned recursively.
• .lua files are sorted before loading.
• Files with # in the name are ignored, matching the previous behavior.
• Folder names are not hardcoded. Renaming npc_CRYSTALSERVER_REV still works.
• Subfolders work automatically.

---

Crystal Compatibility Layer

The compatibility layer in data/npc/lib/crystalcompat/ handles Crystal-style scripts that expect old helper globals or relative dofile() behavior.

Supported behavior:

• Current script directory detection.
• Dynamic dofile() fallback without hardcoded folder names.
• Subfolder support.
• Crystal-style storage helper compatibility.
• Safer NPC event defaults to avoid silent invisible/non-responsive NPCs.
• Automatic support for scripts that define NpcType:new("Name") or modern Game.createNpcType("Name") patterns.

This allows Crystal NPC packs to be imported with minimal edits.

---

NPC Focus and Movement Fix

The NPC movement behavior was normalized across XML NPCs, TFS RevScript NPCs, and Crystal Lua NPCs.

Expected behavior:

1. Player says hi.
2. NPC turns to face the player.
3. NPC stops walking while focused.
4. NPC resumes normal walking only after focus is lost.

Fixed areas:

• Npc::setCreatureFocus clears pending walk directions and stops scheduled walking.
• Npc::onThink avoids random walking while a focused player exists.
• Npc::getNextStep returns early while focused.
• XML NPCs and Crystal Lua NPCs now follow the same focus behavior as modern RevScript NPCs.

---

Missing NPC Diagnostics

When an NPC cannot be found, the engine now reports exactly what was checked:

• No RevScript registration was found in any scanned data/npc/ folder except lib/.
• The fallback XML file data/npc/<name>.xml does not exist.
• The log suggests creating a Lua NPC with NpcType:new("<name>") or checking exact case-sensitive names.

XML loading also now checks file existence before parsing. Missing XML returns false silently as fallback behavior. Malformed XML logs a detailed parser error.

---

Teleport Effect Safety

Game::internalTeleport now accepts an optional MagicEffectClasses argument.

Default behavior is unchanged:

doTeleportThing(cid, pos)

still sends CONST_ME_TELEPORT.

Movement-style teleports can now suppress the blue portal effect:

doTeleportThing(cid, pos, CONST_ME_NONE)
creature:teleportTo(pos, false, CONST_ME_NONE)

This fixes stairs, holes, doors, walkback tiles, and instance rollback scripts showing a fake portal effect.

---

Final Conclusion

The current branch is production-ready from the memory ownership perspective:

• Normal owning raw new/delete: zero.
• Scheduler/Lua raw delete target: resolved; previous counts were grep false positives.
• Player map dangling pointer risk: resolved with weak_ptr<Player>.
• NPC RevScript compatibility: supports TFS, XML fallback, Crystal RevScript folders, renamed folders, and subfolders.
• Valgrind and ASAN validation: clean in user testing.

Remaining T* pointers should be treated as observers unless a future audit proves otherwise.