-
Notifications
You must be signed in to change notification settings - Fork 145
feat(logic): Decouple scripted audio events from CRC computation #2075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(logic): Decouple scripted audio events from CRC computation #2075
Conversation
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle this looks like the right thing to avoid mismatching.
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
|
Does this qualify as a bug? I think it was done intentionally, even if we consider it undesirable. |
|
I do not know. |
|
This still has an open TODO item. |
That function isn't used. My hope was that we could signal that it should not be used with |
|
Ok so what are the next steps for this pull? |
|
#2132 should be merged first, and then this PR should be updated. |
d4fee57 to
bfb09c2
Compare
This reverts commit 81b600a.
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Source/Common/RandomValue.cpp | Added two new "Unchanged" variants of random value functions that use local seed copies to avoid mutating global seed state in non-retail mode |
| Core/GameEngine/Include/GameLogic/LogicRandomValue.h | Added declarations and macros for new "Unchanged" random value functions |
| Core/GameEngine/Source/Common/Audio/AudioEventRTS.cpp | Replaced GameLogicRandomValue with GameLogicRandomValueUnchanged in three locations for logical audio event randomization |
| Core/GameEngine/Source/Common/Audio/GameAudio.cpp | Added conditional compilation to decouple logical audio events from CRC in non-retail mode by allowing early returns |
Sequence Diagram
sequenceDiagram
participant Script as Script Engine
participant AudioMgr as AudioManager
participant AudioEvent as AudioEventRTS
participant Random as Random Functions
participant Seed as Global Seed State
Script->>AudioMgr: addAudioEvent(logicalAudio)
alt RETAIL_COMPATIBLE_CRC enabled
AudioMgr->>AudioMgr: logicalAudio = TRUE
Note over AudioMgr: Cannot take early return<br/>Must process to mutate seed
else Non-retail mode
AudioMgr->>AudioMgr: logicalAudio = FALSE
Note over AudioMgr: Can take early return<br/>if notForLocal
end
AudioMgr->>AudioEvent: generateFilename()
alt Logical Audio
AudioEvent->>Random: GameLogicRandomValueUnchanged()
Note over Random: Creates local seed copy
Random->>Random: randomValue(localSeed)
Note over Seed: Global seed unchanged
Random-->>AudioEvent: random value
else Non-logical Audio
AudioEvent->>Random: GameAudioRandomValue()
Random->>Seed: Mutate audio seed
Random-->>AudioEvent: random value
end
AudioMgr->>AudioEvent: generatePlayInfo()
alt Logical Audio
AudioEvent->>Random: GameLogicRandomValueUnchanged()
Note over Random: Uses local seed copy again
Random-->>AudioEvent: random value
else Non-logical Audio
AudioEvent->>Random: GameAudioRandomValue()
Random-->>AudioEvent: random value
end
alt RETAIL_COMPATIBLE_CRC enabled
alt notForLocal
AudioMgr->>AudioMgr: Release event
AudioMgr-->>Script: AHSV_NotForLocal
Note over AudioMgr: Seed was mutated<br/>CRC stays synced
end
else Non-retail mode
Note over AudioMgr: Already returned early<br/>Seed not mutated
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 2 comments
| if (lo >= hi) | ||
| return hi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: edge case check if (lo >= hi) differs from GetGameLogicRandomValue which checks if (delta == 0) (lines 212-213). When lo == hi, delta is 1 (not 0), so the original proceeds with calculation while this returns early. For true consistency, should check if (delta == 0) or handle lo == hi explicitly
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Source/Common/RandomValue.cpp
Line: 243:244
Comment:
**style:** edge case check `if (lo >= hi)` differs from `GetGameLogicRandomValue` which checks `if (delta == 0)` (lines 212-213). When `lo == hi`, delta is `1` (not `0`), so the original proceeds with calculation while this returns early. For true consistency, should check `if (delta == 0)` or handle `lo == hi` explicitly
How can I resolve this? If you propose a fix, please make it concise.| if (lo >= hi) | ||
| return hi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: edge case check if (lo >= hi) differs from GetGameLogicRandomValueReal which checks if (delta <= 0.0f) (lines 316-317). When lo == hi, delta is 0.0f, so both would return early. However, when lo > hi, delta is negative - the original returns hi, this also returns hi, but the condition phrasing differs which could confuse readers
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngine/Source/Common/RandomValue.cpp
Line: 344:345
Comment:
**style:** edge case check `if (lo >= hi)` differs from `GetGameLogicRandomValueReal` which checks `if (delta <= 0.0f)` (lines 316-317). When `lo == hi`, delta is `0.0f`, so both would return early. However, when `lo > hi`, delta is negative - the original returns `hi`, this also returns `hi`, but the condition phrasing differs which could confuse readers
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.|
Updated with the code from #2132. |
247878f to
ce25f44
Compare
Scripted audio events are synchronized across clients. That means that if the script execution fails for some reason (e.g. caused by another bug), there's a CRC mismatch. This PR changes that, so that scripted audio events are still likely to be the same across clients but script execution failure does not result in a mismatch.
TODO: