Skip to content

Rework the extension system into a mirror class hierarchy#2291

Merged
ZivDero merged 50 commits into
Phobos-developers:developfrom
ZivDero:inherited-extensions-2
Jul 18, 2026
Merged

Rework the extension system into a mirror class hierarchy#2291
ZivDero merged 50 commits into
Phobos-developers:developfrom
ZivDero:inherited-extensions-2

Conversation

@ZivDero

@ZivDero ZivDero commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Reworks the extension system for AbstractClass-derived classes to follow the game's own model, replacing the previous per-class ExtData/ExtMap machinery.

Extension classes now form an inheritance hierarchy mirroring the game's class tree, rooted at AbstractExt (BuildingExt : TechnoExt : RadioExt : MissionExt : ObjectExt : AbstractExt, same on the type side). Every game object carries exactly one extension of its most derived matching type, cached in the unused AbstractClass +0x18 field and fetched via typed Fetch/TryFetch at any hierarchy level.
One container per concrete leaf class tracks live extensions for bulk operations; extensions are allocated/destroyed by ctor/dtor hooks.
Savegame serialization is centralized: per-container streams written after Rules, owners remapped through the swizzle manager, inline slots relinked post-swizzle. The old ~80 per-class savegame hooks are gone.
Pointer invalidation broadcast is replaced by a typed Detach::Listener<T> registry.
Cells are the one exception: the game treats them as value objects with unstable identity (in-place reconstruction, wholesale copies in SetMapDimensions, partial saves), so cell extensions are persisted inline within each cell's own savegame block, and dedicated hooks keep their slots consistent through map setup.

ZivDero added 30 commits July 16, 2026 11:48
Comment thread src/Commands/ObjectInfo.cpp
@TaranDahl

Copy link
Copy Markdown
Contributor

might as well split some variables to children classes (e.g. from TechnoType to Infantry/UnitType) since there're many of them that's only suppose to work for a single type instead of all types

That would be good, but I think it's best to take this in multiple stages (first merge this, then start chipping away at the techno blob separately)

Agree with that.

@NetsuNegi NetsuNegi added the ⚙️T2 T2 maintainer review is sufficient label Jul 17, 2026
@TaranDahl

Copy link
Copy Markdown
Contributor

My agent and I have reviewed all the changes. I believe the following issues require attention:

  1. I noticed the pointer to AbstractExt is placed at the unknown_18 offset instead of the same Dirty offset used by Vinifera (0x20). I do not have detailed knowledge of where unknown_18 is utilized. It may be safer to adopt the corresponding offset consistent with Vinifera. If you confirm this field is never accessed, this will not be a concern.
  2. I am unsure whether RadioExt and MissionExt are necessary, as they are essentially equivalent to parts of TechnoExt.
  3. I noticed you removed the ExtData design. Similar to ExtMap::Find, this modification will trigger extensive code changes and complicate merges with existing branches. I recommend retaining this structure—even though it appears somewhat redundant—to reduce maintenance overhead for other developers.

@ZivDero

ZivDero commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

My agent and I have reviewed all the changes. I believe the following issues require attention:

  1. I noticed the pointer to AbstractExt is placed at the unknown_18 offset instead of the same Dirty offset used by Vinifera (0x20). I do not have detailed knowledge of where unknown_18 is utilized. It may be safer to adopt the corresponding offset consistent with Vinifera. If you confirm this field is never accessed, this will not be a concern.
  2. I am unsure whether RadioExt and MissionExt are necessary, as they are essentially equivalent to parts of TechnoExt.
  3. I noticed you removed the ExtData design. Similar to ExtMap::Find, this modification will trigger extensive code changes and complicate merges with existing branches. I recommend retaining this structure—even though it appears somewhat redundant—to reduce maintenance overhead for other developers.
  1. 0x18 was already used by a large number of extensions (e.g. AnimType), so in that regard it's proven safe. And it's less hassle than Vinifera's IsDirty because the releavnt function doesn't need to be patched.
  2. MissionExt might get use if you choose to add new missions. That said, they mostly exist for completeness. They don't cost anything though, so they're harmless.
  3. Removing ExtData was intentional, and I'd prefer to keep this.

Comment thread src/Ext/Aircraft/Body.cpp Outdated
@TaranDahl

TaranDahl commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Removing the ExtData will blow up most of the current branches 💀

@ZivDero

ZivDero commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Removing the ExtData will blow up most of the current branches 💀

You overestiamte; It should only affect extension header files mostly (for when new fields/methods are added). Most Code already uses auto when fetching is and is thus seamsless. I've added a deprecated alias, though.

@TaranDahl

Copy link
Copy Markdown
Contributor

Removing the ExtData will blow up most of the current branches 💀

You overestiamte; It should only affect extension header files mostly (for when new fields/methods are added). Most Code already uses auto when fetching is and is thus seamsless. I've added a deprecated alias, though.

Yeah but many branches make new fields 🙃

@TaranDahl

Copy link
Copy Markdown
Contributor
image This is the number of branches that need considering migrating cost 🤐

@ZivDero

ZivDero commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

image This is the number of branches that need considering migrating cost 🤐

Surely it won't be a huge burden to move a couple lines in the extension header.

@TaranDahl

Copy link
Copy Markdown
Contributor

Fine. Migration may not be a big problem since we have agent. Maybe I can make another skill to help agent resolve conflicts correctly.

@Coronia

Coronia commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

we've done some intensive FFA test with it, no crash or visible error has occured so far. Is there anything that should be tested specifically?

@ZivDero

ZivDero commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

we've done some intensive FFA test with it, no crash or visible error has occured so far. Is there anything that should be tested specifically?

I was just doing some final tests and discovered that loading a save from the client works, but loading from inside the game crashes (I was unable to test this previously due to ddraw breaking the menus). I'm looking into that, and once that's resolved, it should be good to go.

@ZivDero

ZivDero commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Fixed; Should be good to go

@ZivDero
ZivDero merged commit 2cb961e into Phobos-developers:develop Jul 18, 2026
7 checks passed
DeathFishAtEase added a commit to DeathFishAtEase/Phobos that referenced this pull request Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

❓Infrastructure Related to docs, build system, CI/CD etc. No Documentation Needed No documentation needed whatsoever ⚙️T2 T2 maintainer review is sufficient

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants