Skip to content
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

[Issue #9568] Replace RRC lift sounds LiftWood to match SFRC #13071

Merged

Conversation

iking96
Copy link
Contributor

@iking96 iking96 commented Oct 2, 2020

Description

Proposed solution to #9568. Updates Reverser RC to have same lift sound as the Side Friction RC. Updates Compact Inverted Coaster to have the classic lift sound.

Ride Current sound Proposal
SLC Friction wheels Classic chain sound or Arrow sound
Reverser RC Classic chain sound Side-friction RC sound

@iking96 iking96 changed the title refactor: Replace RRC lift sounds LiftWood to match SFRC [Issue #9568] Replace RRC lift sounds LiftWood to match SFRC Oct 2, 2020
@iking96 iking96 force-pushed the iking96_update_RRC_and_SLC_lift_sounds branch from 0e0d186 to 26cd319 Compare October 2, 2020 23:37
@iking96
Copy link
Contributor Author

iking96 commented Oct 3, 2020

Hey @Gymnasiast,

I am a having a bit of trouble debugging this failure: https://github.com/OpenRCT2/OpenRCT2/runs/1200743771

...
WARNING[../src/openrct2/ReplayManager.cpp:736 (Serialise)]: Replay network version mismatch: '0.2.6-25', expected: '0.3.1-0'
WARNING[../src/openrct2/ReplayManager.cpp:806 (CheckState)]: Different sprite checksum at tick 84547 (Replay Tick: 633) ; Saved: 33608acac467a02c8df21673c6a6a09eb22cc93d, Current: 814ea9f3f6130fc2c85b6c70de4aba46df8c4a33
For security reasons, it is not recommended to run OpenRCT2 with elevated permissions.
../test/tests/ReplayTests.cpp:101: Failure
Value of: replayManager->IsPlaybackStateMismatching() == false
  Actual: false
Expected: true
[  FAILED  ] Replay/ReplayTests.RunReplay/bpb1000Ticks, where GetParam() = testdata/replays/bpb1000Ticks.sv6r (3104 ms)
...

I believe the failure is saying this method returned false:

virtual bool IsPlaybackStateMismatching() const override
{
if (_mode != ReplayMode::PLAYING)
{
return false;
}
return _faultyChecksumIndex != -1;
}

Judging by the warning proceeding the failure I believe I am hitting this branch:

if (savedChecksum.second.raw != checksum.raw)
{
uint32_t replayTick = gCurrentTicks - _currentReplay->tickStart;
// Detected different game state.
log_warning(
"Different sprite checksum at tick %u (Replay Tick: %u) ; Saved: %s, Current: %s", gCurrentTicks,
replayTick, savedChecksum.second.ToString().c_str(), checksum.ToString().c_str());
_faultyChecksumIndex = checksumIndex;
}

I think this test is re-running game scenarios and ensuring sprites are unchanged vs some golden file. I suppose the "LiftData" counts as part of the sprite and could throw its checksum off; is that your understanding as well?

What is the best way to move forward here, would I need to update something in this repo: https://github.com/OpenRCT2/replays?

@duncanspumpkin
Copy link
Contributor

Replays just need to be rerecorded. I'm normally the one who does that but I won't do that until it's confirmed this fix is good to go

@Gymnasiast
Copy link
Member

I'll try to review this this weekend.

Copy link
Member

@Gymnasiast Gymnasiast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is good.

Could you add a changelog entry?
Change: [#9568] Change lift sounds of Reverser Roller Coaster and Compact Inverted Coaster to better fitting ones. Put it between the FeatureandFix` entries.

Replays will need to be rerecorded. @duncanspumpkin could you handle that?

@iking96 iking96 force-pushed the iking96_update_RRC_and_SLC_lift_sounds branch from 7395b97 to a7b06bb Compare October 4, 2020 17:42
@tupaschoal
Copy link
Member

Just needs replay rerecording now

duncanspumpkin added a commit to OpenRCT2/replays that referenced this pull request Oct 5, 2020
@duncanspumpkin
Copy link
Contributor

Replay has been rerecorded. You'll need to correct the conflict in the changelog.

@Gymnasiast
Copy link
Member

@iking96 Could you rebase, rather than merge?

@iking96 iking96 force-pushed the iking96_update_RRC_and_SLC_lift_sounds branch from 18fb730 to aec8c9b Compare October 5, 2020 20:53
@iking96
Copy link
Contributor Author

iking96 commented Oct 5, 2020

@iking96 Could you rebase, rather than merge?

@Gymnasiast Sorry about that, done!

@tupaschoal tupaschoal merged commit fb47b3b into OpenRCT2:develop Oct 6, 2020
@tupaschoal tupaschoal added this to the v0.3.2 milestone Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants