Plat 11607 modular ship assembly - #62
Conversation
The new base class has support "owner", "parent" and "part tag" shared attributes.
Use `UnregisterChild` helper method Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The new base class has support "owner", "parent" and "part tag" shared attributes.
Use `UnregisterChild` helper method Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…rbonengine/trinity into space-object-child-base-class
… already doing it
There was a problem hiding this comment.
Pull request overview
Adds support for modular ship/space-object assembly by tagging parts, building SOF children as discrete parts, and enabling runtime add/remove/transform operations while keeping instanced meshes and locator sets attributable to the originating part.
Changes:
- Introduces
PartTagpropagation through locators and shared instanced meshes to track ownership per modular part. - Adds
EveSOF::BuildChild(...)plus related plumbing to build additional hull parts into an existingEveSpaceObject2. - Adds
EveChildPartData+EveModularObjectModifier(and Blue bindings) to create/modify modular objects (add/remove parts, update transforms).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| trinity/Eve/SpaceObjectFactory/EveSOFDataMgr.h | Extends locator direction data with partTag and forwards it into Locator. |
| trinity/Eve/SpaceObjectFactory/EveSOF.h | Declares BuildChild and extends SetupLocatorSets to accept an optional partTag. |
| trinity/Eve/SpaceObjectFactory/EveSOF.cpp | Implements BuildChild, propagates partTag into locators, and adjusts placement/attachment behavior. |
| trinity/Eve/SpaceObject/Utils/EveLocatorSets.h | Adds partTag to Locator and provides non-const locator list access. |
| trinity/Eve/SpaceObject/Utils/EveLocatorSets.cpp | Updates structure definition to serialize/reflect partTag and adds non-const accessor impl. |
| trinity/Eve/SpaceObject/EveSpaceObject2.h | Exposes mutable accessors for effect children and locator sets, plus public bounding-sphere getters. |
| trinity/Eve/SpaceObject/Children/EveSpaceObjectChild.cpp | Loosens an assertion when unregistering a child with no parent. |
| trinity/Eve/SpaceObject/Children/EveChildPartData.h | Adds new child type for per-object part bookkeeping and a modular-object modifier API. |
| trinity/Eve/SpaceObject/Children/EveChildPartData.cpp | Implements modular-object creation/modification (add/remove parts, transform updates, locator cleanup). |
| trinity/Eve/SpaceObject/Children/EveChildPartData_Blue.cpp | Exposes modular-object APIs to Blue. |
| trinity/Eve/SpaceObject/Children/EveChildInstancedMeshes.h | Extends AddMesh with partTag, adds per-instance part tag storage and removal API. |
| trinity/Eve/SpaceObject/Children/EveChildInstancedMeshes.cpp | Implements part-tag-aware merging and removal of instanced mesh instances. |
| trinity/Eve/EveInstancedMeshManager.h | Adds move-assignment for DataHandle used by instanced-mesh registration handles. |
| trinity/CMakeLists.txt | Adds new modular-object source files to the build. |
Suppressed comments (1)
trinity/Eve/SpaceObject/Children/EveChildInstancedMeshes.cpp:570
- After removing instances, the remaining instances keep their previous sphereIndex values. If removals happen in the middle, sphereIndex can become out-of-range for the resized bounding sphere group, breaking frustum culling/picking.
mesh.instances.erase( newEnd, end( mesh.instances ) );
auto newTagEnd = std::remove_if( begin( mesh.partTags ), end( mesh.partTags ), [&]( uint32_t tag ) {
return tag == partTag;
} );
mesh.partTags.erase( newTagEnd, end( mesh.partTags ) );
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Cpp-linter Review
Used clang-format v21.1.8
Click here for the full clang-format patch
diff --git a/trinity/Eve/SpaceObject/Utils/EveLocatorSets.cpp b/trinity/Eve/SpaceObject/Utils/EveLocatorSets.cpp
index d308673..33be1c0 100644
--- a/trinity/Eve/SpaceObject/Utils/EveLocatorSets.cpp
+++ b/trinity/Eve/SpaceObject/Utils/EveLocatorSets.cpp
@@ -8 +8 @@ static_assert( sizeof( EveSpaceObjectChild::PartTag ) == sizeof( uint32_t ), "Si
- // locator item definition
+// locator item definition
diff --git a/trinity/Eve/SpaceObjectFactory/EveSOF.h b/trinity/Eve/SpaceObjectFactory/EveSOF.h
index 892b335..367e136 100644
--- a/trinity/Eve/SpaceObjectFactory/EveSOF.h
+++ b/trinity/Eve/SpaceObjectFactory/EveSOF.h
@@ -48 +48 @@ public:
- bool BuildChild( EveSpaceObject2* owner, const char* dnaString, uint32_t partTag, const Matrix& transform, uint32_t& nextPartTag );
+ bool BuildChild( EveSpaceObject2 * owner, const char* dnaString, uint32_t partTag, const Matrix& transform, uint32_t& nextPartTag );
Have any feedback or feature suggestions? Share it here.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Cpp-linter Review
Used clang-format v21.1.8
Click here for the full clang-format patch
diff --git a/trinity/Eve/SpaceObjectFactory/EveSOF.h b/trinity/Eve/SpaceObjectFactory/EveSOF.h
index 892b335..367e136 100644
--- a/trinity/Eve/SpaceObjectFactory/EveSOF.h
+++ b/trinity/Eve/SpaceObjectFactory/EveSOF.h
@@ -48 +48 @@ public:
- bool BuildChild( EveSpaceObject2* owner, const char* dnaString, uint32_t partTag, const Matrix& transform, uint32_t& nextPartTag );
+ bool BuildChild( EveSpaceObject2 * owner, const char* dnaString, uint32_t partTag, const Matrix& transform, uint32_t& nextPartTag );
Have any feedback or feature suggestions? Share it here.
|
It would be good to start adding documentation for new features |
|
|
||
|
|
||
| BLUE_CLASS_IMPL( EveModularObjectModifier ); | ||
| class EveModularObjectModifier : public IRoot |
There was a problem hiding this comment.
Can we move this into a separate file(s)? According to the coding standard, one class per file-ish.
| const Be::ClassInfo* EveModularObjectModifier::ExposeToBlue() | ||
| { | ||
| EXPOSURE_BEGIN( EveModularObjectModifier, "" ) | ||
| MAP_METHOD_AND_WRAP( "AddHull", AddHull, "" ); |
| SetupAudio( BlueCastPtr( placementContainer ), dna, transform ); | ||
|
|
||
|
|
||
| //SetupInstancedMeshes( newObj, dna, placementOffsets ); |
There was a problem hiding this comment.
Commended code. Maybe add a comment instead that "old-style instanced meshes are not supported here"
| SetupEffects( newObj, (IEveEffectChildrenOwnerPtr)placementContainer, dna, placementOffsets, buildFlags ); | ||
| } | ||
|
|
||
| //SetupCustomMask( newObj, dna ); |
| { | ||
| std::vector<const EveChildPartData::PartData*> orderedParts; | ||
| orderedParts.reserve( m_data->m_parts.size() ); | ||
| for( const auto& part : m_data->m_parts ) |
| SetupAudio( BlueCastPtr( placementContainer ), dna, transform ); | ||
|
|
||
|
|
||
| //SetupInstancedMeshes( newObj, dna, placementOffsets ); |
| SetupEffects( newObj, (IEveEffectChildrenOwnerPtr)placementContainer, dna, placementOffsets, buildFlags ); | ||
| } | ||
|
|
||
| //SetupCustomMask( newObj, dna ); |
| if( owner ) | ||
| { | ||
| owner->ReplaceHandle( this, &other ); | ||
| owner->ReplaceHandle( &other, this ); |
There was a problem hiding this comment.
this makes me nervous for some reason. Why is this reversed?
Does it have something to do with the fact that the owner and index swap is happening before this call now?
And doesn’t that mean that this and other is the same (value wise)?
Creating a system for building up a ship or otherwise from components and handling their attachments like locators