Update Booster persistance - #55
Open
Selph wants to merge 18 commits into
Open
Conversation
We've been having trouble preserving booster data with red file loading. A common scenario is when a user loads a scene state. The boosters on the ships in those scenes would be dead since the sof locator hookup required to set up the boosters correctly is missing. We've been compensating for this by employing EveBoosterSet2.RebuildBoosterSet. This method however doesn't preserve the data from the booster, and sometimes ends up setting wrong atlasIndices for instance which can majorly affect the booster visual. So we'd end up in a scenario where we can't load scenes properly through trinity.Load because there's no feasible way to make the sof connection through that avenue, something that LoadFromDNA has no trouble doing. The proposed solution to this is to persist the m_singleBoosters data and make it public. This way it can be serialized upon save and smoothly loaded back up when loading a scene. Some of the params there are non-desirable to persist, being randomized per instance, which was accounted for here. `RebuildBoosterSet` was also modified to persist the relevant data from the old booster data (effect, glows, trails, visual settings) when rebuilding the boosters.
- boosters attribute: keep Be::READ | Be::PERSIST (not PERSISTONLY) - platformtools' make-compatible-with-trinity-booster-update branch detects this feature via hasattr(trinity.EveBoosterSet2, "boosters"), which needs the attribute visible to Python; WRITE stays withheld since nothing should reassign the whole list from Python. - Move RebuildBoosterSet's snapshot/restore/rebuild logic into a single EveBoosterSet2::RebuildBoosters(locatorTransforms), so EveShip2 just gathers locator transforms and delegates. - Remove IBlueStructureListNotify: it fired unconditionally on every internal Append(), causing Add() to redundantly reprocess the whole list on every call and leaving m_runtimeLights permanently one entry longer than m_boosters after the last Add() in a batch (a real duplicated glow/trail/phantom-light bug). Add() alone is sufficient once nothing external can mutate the list. - Drop SnapshotPersistedItems() as a separate public method (folded inline). - Stop persisting transform (the ship owns locator authority); move the rebuild-from-locators responsibility to EveShip2::Initialize(), mirroring EveMobile::Initialize()'s unconditional call to RebuildTurretPositions(). - Fix RebuildPreservingSettings() not resetting m_maxSize, found while consolidating the two rebuild paths into one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…o Python Keep "boosters" as Be::PERSISTONLY per review feedback -- there's no reason for Python to see the per-item art data. platformtools' upcoming make-compatible-with-trinity-booster-update branch needs to detect this build though (to skip a redundant RebuildBoosterSet() call and hide its now-unnecessary "Fix Boosters" Jessica action), so add a minimal read-only hasPersistentBoosterItems property whose only job is that detection -- it exposes a flag, not the underlying data. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.cpp # trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.h
|
Anonymous CCP Employee seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cpp-linter Review
Used clang-format v20.1.2
Click here for the full clang-format patch
diff --git a/trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.cpp b/trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.cpp
index 7028b2c..7faf320 100644
--- a/trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.cpp
+++ b/trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.cpp
@@ -51,11 +51,10 @@ namespace
- BlueStructureDefinition s_boosterItemStructureDef[] =
- {
- { "functionality", Be::FLOAT32_4, offsetof( EveBoosterItem, functionality ) },
- { "atlasIndex0", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex0 ) },
- { "atlasIndex1", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex1 ) },
- { "hasTrail", Be::INT32_1, offsetof( EveBoosterItem, hasTrail ) },
- { "lightScale", Be::FLOAT32_1, offsetof( EveBoosterItem, lightScale ) },
- { 0 }
- };
-
- EveBoosterItem s_defaultBoosterItem;
+BlueStructureDefinition s_boosterItemStructureDef[] = {
+ { "functionality", Be::FLOAT32_4, offsetof( EveBoosterItem, functionality ) },
+ { "atlasIndex0", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex0 ) },
+ { "atlasIndex1", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex1 ) },
+ { "hasTrail", Be::INT32_1, offsetof( EveBoosterItem, hasTrail ) },
+ { "lightScale", Be::FLOAT32_1, offsetof( EveBoosterItem, lightScale ) },
+ { 0 }
+};
+
+EveBoosterItem s_defaultBoosterItem;
@@ -65,2 +64,2 @@ namespace
-EveBoosterItem::EveBoosterItem()
- : transform( IdentityMatrix() ),
+EveBoosterItem::EveBoosterItem() :
+ transform( IdentityMatrix() ),
@@ -933 +932 @@ void EveBoosterSet2::Add( const Matrix* localMatrix, const Vector4* functionalit
- item.transform = *localMatrix;
+ item.transform = *localMatrix;
@@ -935,4 +934,4 @@ void EveBoosterSet2::Add( const Matrix* localMatrix, const Vector4* functionalit
- item.atlasIndex0 = atlasIndex0;
- item.atlasIndex1 = atlasIndex1;
- item.hasTrail = hasTrail ? 1 : 0;
- item.lightScale = lightScale;
+ item.atlasIndex0 = atlasIndex0;
+ item.atlasIndex1 = atlasIndex1;
+ item.hasTrail = hasTrail ? 1 : 0;
+ item.lightScale = lightScale;
@@ -983,2 +982,2 @@ void EveBoosterSet2::ComputeBoosterLight( const EveBoosterItem& item, BoosterLig
- out.radius = std::max( Length( item.transform.GetX() ), Length( item.transform.GetY() ) ) * item.lightScale;
- out.phase = float( g_lightNoiseSize ) * float( rand() ) / float( RAND_MAX );
+ out.radius = std::max( Length( item.transform.GetX() ), Length( item.transform.GetY() ) ) * item.lightScale;
+ out.phase = float( g_lightNoiseSize ) * float( rand() ) / float( RAND_MAX );
@@ -1019,4 +1018,4 @@ void EveBoosterSet2::RebuildBoosters( const std::vector<Matrix>& locatorTransfor
- hasTrail = saved.hasTrail != 0;
- atlasIndex0 = saved.atlasIndex0;
- atlasIndex1 = saved.atlasIndex1;
- lightScale = saved.lightScale;
+ hasTrail = saved.hasTrail != 0;
+ atlasIndex0 = saved.atlasIndex0;
+ atlasIndex1 = saved.atlasIndex1;
+ lightScale = saved.lightScale;
diff --git a/trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.h b/trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.h
index a638b8f..1cc4f7a 100644
--- a/trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.h
+++ b/trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.h
@@ -48,6 +48,6 @@ struct EveBoosterItem
- Matrix transform;
- Vector4 functionality;
- uint32_t atlasIndex0;
- uint32_t atlasIndex1;
- int32_t hasTrail;
- float lightScale;
+ Matrix transform;
+ Vector4 functionality;
+ uint32_t atlasIndex0;
+ uint32_t atlasIndex1;
+ int32_t hasTrail;
+ float lightScale;
@@ -273,2 +273,2 @@ public:
- float radius;
- float phase;
+ float radius;
+ float phase;
@@ -290 +290,4 @@ public:
- bool HasPersistentBoosterItems() const { return true; }
+ bool HasPersistentBoosterItems() const
+ {
+ return true;
+ }
@@ -340 +343 @@ private:
- void CreateFlares( const EveBoosterItem & item );
+ void CreateFlares( const EveBoosterItem& item );
Have any feedback or feature suggestions? Share it here.
Comment on lines
+51
to
+61
| BlueStructureDefinition s_boosterItemStructureDef[] = | ||
| { | ||
| { "functionality", Be::FLOAT32_4, offsetof( EveBoosterItem, functionality ) }, | ||
| { "atlasIndex0", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex0 ) }, | ||
| { "atlasIndex1", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex1 ) }, | ||
| { "hasTrail", Be::INT32_1, offsetof( EveBoosterItem, hasTrail ) }, | ||
| { "lightScale", Be::FLOAT32_1, offsetof( EveBoosterItem, lightScale ) }, | ||
| { 0 } | ||
| }; | ||
|
|
||
| EveBoosterItem s_defaultBoosterItem; |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| BlueStructureDefinition s_boosterItemStructureDef[] = | |
| { | |
| { "functionality", Be::FLOAT32_4, offsetof( EveBoosterItem, functionality ) }, | |
| { "atlasIndex0", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex0 ) }, | |
| { "atlasIndex1", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex1 ) }, | |
| { "hasTrail", Be::INT32_1, offsetof( EveBoosterItem, hasTrail ) }, | |
| { "lightScale", Be::FLOAT32_1, offsetof( EveBoosterItem, lightScale ) }, | |
| { 0 } | |
| }; | |
| EveBoosterItem s_defaultBoosterItem; | |
| BlueStructureDefinition s_boosterItemStructureDef[] = { | |
| { "functionality", Be::FLOAT32_4, offsetof( EveBoosterItem, functionality ) }, | |
| { "atlasIndex0", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex0 ) }, | |
| { "atlasIndex1", Be::UINT32_1, offsetof( EveBoosterItem, atlasIndex1 ) }, | |
| { "hasTrail", Be::INT32_1, offsetof( EveBoosterItem, hasTrail ) }, | |
| { "lightScale", Be::FLOAT32_1, offsetof( EveBoosterItem, lightScale ) }, | |
| { 0 } | |
| }; | |
| EveBoosterItem s_defaultBoosterItem; |
Comment on lines
+65
to
+66
| EveBoosterItem::EveBoosterItem() | ||
| : transform( IdentityMatrix() ), |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| EveBoosterItem::EveBoosterItem() | |
| : transform( IdentityMatrix() ), | |
| EveBoosterItem::EveBoosterItem() : | |
| transform( IdentityMatrix() ), |
| m_singleBoosters.push_back( sbd ); | ||
| // keep source data for persistence | ||
| EveBoosterItem item; | ||
| item.transform = *localMatrix; |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| item.transform = *localMatrix; | |
| item.transform = *localMatrix; |
Comment on lines
+935
to
+938
| item.atlasIndex0 = atlasIndex0; | ||
| item.atlasIndex1 = atlasIndex1; | ||
| item.hasTrail = hasTrail ? 1 : 0; | ||
| item.lightScale = lightScale; |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| item.atlasIndex0 = atlasIndex0; | |
| item.atlasIndex1 = atlasIndex1; | |
| item.hasTrail = hasTrail ? 1 : 0; | |
| item.lightScale = lightScale; | |
| item.atlasIndex0 = atlasIndex0; | |
| item.atlasIndex1 = atlasIndex1; | |
| item.hasTrail = hasTrail ? 1 : 0; | |
| item.lightScale = lightScale; |
Comment on lines
+983
to
+984
| out.radius = std::max( Length( item.transform.GetX() ), Length( item.transform.GetY() ) ) * item.lightScale; | ||
| out.phase = float( g_lightNoiseSize ) * float( rand() ) / float( RAND_MAX ); |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| out.radius = std::max( Length( item.transform.GetX() ), Length( item.transform.GetY() ) ) * item.lightScale; | |
| out.phase = float( g_lightNoiseSize ) * float( rand() ) / float( RAND_MAX ); | |
| out.radius = std::max( Length( item.transform.GetX() ), Length( item.transform.GetY() ) ) * item.lightScale; | |
| out.phase = float( g_lightNoiseSize ) * float( rand() ) / float( RAND_MAX ); |
Comment on lines
+1019
to
+1022
| hasTrail = saved.hasTrail != 0; | ||
| atlasIndex0 = saved.atlasIndex0; | ||
| atlasIndex1 = saved.atlasIndex1; | ||
| lightScale = saved.lightScale; |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| hasTrail = saved.hasTrail != 0; | |
| atlasIndex0 = saved.atlasIndex0; | |
| atlasIndex1 = saved.atlasIndex1; | |
| lightScale = saved.lightScale; | |
| hasTrail = saved.hasTrail != 0; | |
| atlasIndex0 = saved.atlasIndex0; | |
| atlasIndex1 = saved.atlasIndex1; | |
| lightScale = saved.lightScale; |
Comment on lines
+48
to
+53
| Matrix transform; | ||
| Vector4 functionality; | ||
| uint32_t atlasIndex0; | ||
| uint32_t atlasIndex1; | ||
| int32_t hasTrail; | ||
| float lightScale; |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| Matrix transform; | |
| Vector4 functionality; | |
| uint32_t atlasIndex0; | |
| uint32_t atlasIndex1; | |
| int32_t hasTrail; | |
| float lightScale; | |
| Matrix transform; | |
| Vector4 functionality; | |
| uint32_t atlasIndex0; | |
| uint32_t atlasIndex1; | |
| int32_t hasTrail; | |
| float lightScale; |
Comment on lines
+273
to
+274
| float radius; | ||
| float phase; |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| float radius; | |
| float phase; | |
| float radius; | |
| float phase; |
| // HasPersistentBoosterItems()). Always true on any build where this is exposed to Blue. | ||
| // Deliberately exposes only this flag, not the per-booster data itself, which stays | ||
| // Be::PERSISTONLY (see the "boosters" MAP_ATTRIBUTE in EveBoosterSet2_Blue.cpp). | ||
| bool HasPersistentBoosterItems() const { return true; } |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| bool HasPersistentBoosterItems() const { return true; } | |
| bool HasPersistentBoosterItems() const | |
| { | |
| return true; | |
| } |
|
|
||
| // function to create the flares from boosterdata | ||
| void CreateFlares( SingleBoosterData & boosterData ); | ||
| void CreateFlares( const EveBoosterItem & item ); |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| void CreateFlares( const EveBoosterItem & item ); | |
| void CreateFlares( const EveBoosterItem& item ); |
There was a problem hiding this comment.
Pull request overview
This PR updates Trinity’s ship booster system so per-booster art/config data (functionality, atlas indices, trail flag, light scale) persists across .red save/load instead of being re-derived from locators each load, fixing incorrect booster setup when loading without SOF.
Changes:
- Introduces persisted per-booster metadata via
EveBoosterItem(BLUE_DECLARE_STRUCTURE_LIST) and keeps derived point-light data in a parallel non-persisted array. - Adds
EveBoosterSet2::RebuildBoosters(locatorTransforms)as the single rebuild entry point, restoring persisted values by index with fallback defaults. - Updates
EveShip2to gatherlocator_booster*transforms and rebuild boosters duringInitialize(), plus exposes a Blue feature-detection marker (hasPersistentBoosterItems).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| trinity/Eve/SpaceObject/EveShip2.cpp | Rebuilds boosters from ship locator transforms and triggers rebuild during ship initialization. |
| trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.h | Adds persisted EveBoosterItem, a derived BoosterLight, and the RebuildBoosters API plus feature marker. |
| trinity/Eve/SpaceObject/Attachments/EveBoosterSet2.cpp | Implements persisted booster list + derived light list, rebuild logic, and updates rendering/light paths to use new storage. |
| trinity/Eve/SpaceObject/Attachments/EveBoosterSet2_Blue.cpp | Exposes persisted booster list as PERSISTONLY and adds read-only compatibility marker property. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+888
to
+896
| // -------------------------------------------------------------------------------- | ||
| // Description: | ||
| // Rebuild boosters while preserving effects, glows, trails and visual settings. | ||
| // This is used when locators change but the booster configuration should remain. | ||
| // -------------------------------------------------------------------------------- | ||
| void EveBoosterSet2::RebuildPreservingSettings() | ||
| { | ||
| // Clear only the booster items, not the effects/glows/trails | ||
| m_boosters.Clear(); |
Comment on lines
866
to
886
| @@ -848,27 +885,70 @@ void EveBoosterSet2::Clear() | |||
| ReleaseResources( TRISTORAGE_ALL ); | |||
| } | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Per-booster data (functionality, atlas indices, trail flag, light scale) was derived from locators on every load, so boosters could not be set up correctly when loading .red files without SOF. EveShip2.RebuildBoosterSet was the workaround in Graphite and Jessica before it, but it only applied defaults, so boosters came back with wrong atlas indices, circular instead of triangular and so on.
This persists that data as a StructureList on EveBoosterSet2.
Since the last review round: dropped OnStructureListModified, stopped persisting transform, moved restore-by-index into EveBoosterSet2, replaced the Python exposure with the marker property, and renamed RuntimeLightData to BoosterLight.
Old .red files load an empty list and fall back to locator defaults, identical to current behavior. Native and Python callers of RebuildBoosterSet are unaffected.
Companion PR: platformtools make-compatible-with-trinity-booster-update.
Testing done: persistence across save/load, ships booting with working boosters, copy/paste and duplicate in Graphite.
Addresses https://ccpgames.atlassian.net/browse/PLAT-9555