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

calculate ticks from ms instead of seconds #5755

Merged
merged 24 commits into from
Jul 7, 2017
Merged

calculate ticks from ms instead of seconds #5755

merged 24 commits into from
Jul 7, 2017

Conversation

spacek531
Copy link
Contributor

@spacek531 spacek531 commented Jul 2, 2017

I have very little experience with statically-typed languages so YMMV with the sint32.

I added the Min because Update() appears to be incapable of handling negative _waitCounter values.

There used to be a Ceil function in there but I changed my mind because I realized that it probably casts it to an int.

See also: OpenRCT2/title-sequences#1

@spacek531
Copy link
Contributor Author

I notice the title sequence pr was merged. Any reason this one has not been merged? I hope this code isn't too awful.

@@ -247,7 +247,9 @@ class TitleSequencePlayer final : public ITitleSequencePlayer
_waitCounter = 1;
break;
case TITLE_SCRIPT_WAIT:
_waitCounter = command->Seconds * UPDATE_FPS;

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty line

@@ -247,7 +247,9 @@ class TitleSequencePlayer final : public ITitleSequencePlayer
_waitCounter = 1;
break;
case TITLE_SCRIPT_WAIT:
_waitCounter = command->Seconds * UPDATE_FPS;

// The waitCounter is measured in 25-ms game ticks. Previously it was seconds * 40 ticks/second, now it is ms / 25 ms/tick
Copy link
Contributor

Choose a reason for hiding this comment

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

Change tabs to spaces

@@ -48,4 +48,5 @@ namespace Math
if (x > 0) return 1;
return 0;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Undo this change.

@IntelOrca
Copy link
Contributor

  • TitleCommand.Seconds needs renaming.
  • The string for wait command in seconds needs changing.
  • The default value for wait command should be 10000.

@spacek531
Copy link
Contributor Author

I do not know how to say "milliseconds" in all the languages. Hopefully the localisation team can be roused.

@@ -1,4 +1,4 @@
# STR_XXXX part is read and XXXX becomes the string id number.
# STR_XXXX part is read and XXXX becomes the string id number.
Copy link
Member

Choose a reason for hiding this comment

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

MS has the annoying habit of inserting BOMs into UTF-8 files. Please remove it.

@IntelOrca
Copy link
Contributor

IntelOrca commented Jul 6, 2017

This will be merged at the same time the title-sequences get its new release which includes the new title sequence.

I will handle the merge.

@IntelOrca IntelOrca self-assigned this Jul 6, 2017
@@ -247,7 +247,8 @@ class TitleSequencePlayer final : public ITitleSequencePlayer
_waitCounter = 1;
break;
case TITLE_SCRIPT_WAIT:
_waitCounter = command->Seconds * UPDATE_FPS;
// The waitCounter is measured in 25-ms game ticks. Previously it was seconds * 40 ticks/second, now it is ms / 25 ms/tick
_waitCounter = Math::Min<sint32>(1, command->Milliseconds / UPDATE_TIME_MS);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be Math::Max

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must have had a really bad case of the dumb four days ago.

@spacek531
Copy link
Contributor Author

This will also solve issue #2438

@spacek531
Copy link
Contributor Author

note that unless changing Min to Max magically changes the way things work, this pr does not currently work. I tested the last build and the script did not seem to advance - the camera was stuck in the first position seemingly indefinitely.

@spacek531
Copy link
Contributor Author

spacek531 commented Jul 6, 2017

I just tested it and changing Min to Max has changed things significantly. Currently copying a script from the ms-updated scripts to test.

@spacek531
Copy link
Contributor Author

I just tested the current sequence with the ms-updated times. Fabulous!

@spacek531
Copy link
Contributor Author

spacek531 commented Jul 6, 2017

Using the ingame editor for ms is infeasible right now because it is limited to 3 digits, when it should be limited to 5 digits.

Also, this saves and loads as 12:
image

@spacek531
Copy link
Contributor Author

spacek531 commented Jul 6, 2017

sometimes I think the reason github is confusing is because the github gui does nothing but confuse people. It can't even git properly.

@spacek531
Copy link
Contributor Author

spacek531 commented Jul 6, 2017

Do we want to merge this sooner and get the localisation team on it before 0.1.0 or get the localisations in 0.1.0 develop?

@spacek531
Copy link
Contributor Author

spacek531 commented Jul 7, 2017

apparently I can't just change a type in a structure to include more bits.

Apparently I need a proper number of curly brackets

@spacek531
Copy link
Contributor Author

spacek531 commented Jul 7, 2017

there is some kind of uint8 bottleneck going both ways between the command list and the text box from the command list to the text box and I'm out of my depth
image

@spacek531
Copy link
Contributor Author

The movement of the value between file and text box is working fine, but now it seems the number is displaying as a sint16 instead of int16:
image
I looked in here but didn't see anything that jumped out at me as to why.
https://github.com/OpenRCT2/OpenRCT2/blob/develop/src/openrct2/windows/title_editor.c#L837

@spacek531
Copy link
Contributor Author

No matter what I do I cannot seem to reformat that string using the language files.

@spacek531
Copy link
Contributor Author

I think this is the final version, boys.

At least, I really hope so because once I get the title sequence PR going I'm going to bed.

@@ -241,9 +242,11 @@ void window_title_command_editor_open(TitleSequence * sequence, sint32 index, bo
snprintf(textbox2Buffer, BUF_SIZE, "%d", command.Y);
break;
case TITLE_SCRIPT_ROTATE:
Copy link
Member

Choose a reason for hiding this comment

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

this should also handle the ZOOM case as it did before.

@spacek531
Copy link
Contributor Author

spacek531 commented Jul 7, 2017

celebration in progress

OpenRCT2/title-sequences#2

OpenRCT2/Localisation#1145

@IntelOrca IntelOrca merged commit 22a936b into OpenRCT2:develop Jul 7, 2017
@spacek531 spacek531 mentioned this pull request Jul 7, 2017
14 tasks
janisozaur added a commit that referenced this pull request Jul 12, 2017
- Feature: [#1399 (partial), #5177] Add window that displays any missing/corrupt objects when loading a park
- Feature: [#5056] Add cheat to own all land.
- Feature: [#5133] Add option to display guest expenditure (as seen in RCTC).
- Feature: [#5196] Add cheat to disable ride ageing.
- Feature: [#5504] Group vehicles into ride groups
- Feature: [#5576] Add a persistent 'display real names of guests' setting.
- Feature: [#5611] Add support for Android
- Feature: [#5706] Add support for OpenBSD
- Feature: OpenRCT2 now starts up on the display it was last shown on.
- Feature: Park entrance fee can now be set to amounts up to £200.
- Improved: Construction rights can now be placed on park entrances.
- Improved: Mouse can now be dragged to select scenery when saving track designs
- Fix: [#259] Money making glitch involving swamps (original bug)
- Fix: [#441] Construction rights over entrance path erased (original bug)
- Fix: [#578] Ride ghosts show up in ride list during construction (original bug)
- Fix: [#597] 'Finish 5 roller coasters' goal not properly checked (original bug)
- Fix: [#667] Incorrect banner limit calculation (original bug)
- Fix: [#739] Crocodile Ride (Log Flume) never allows more than five boats (original bug)
- Fix: [#837] Can't move windows on title screen to where the toolbar would be (original bug)
- Fix: [#1705] Time Twister's Medieval entrance has incorrect scrolling (original bug)
- Fix: [#3178, #5456] Paths with non-ASCII characters not handled properly on macOS.
- Fix: [#3346] Crash when extra long train breaks down at the back
- Fix: [#3479] Building in pause mode creates too many floating numbers, crashing the game
- Fix: [#3565] Multiplayer server crash
- Fix: [#3681] Steel Twister rollercoaster always shows all track designs
- Fix: [#3846, #5749] Crash when testing coaster with a diagonal lift in block brake mode
- Fix: [#4054] Sorting rides by track type: Misleading research messages
- Fix: [#4055] Sort rides by track type: Sorting rule is not really clear (inconsistent?)
- Fix: [#4512] Invisible map edge tiles corrupted
- Fix: [#5009] Ride rating calculations can overflow
- Fix: [#5253] RCT1 park value conversion factor too high
- Fix: [#5400] New Ride window does not focus properly on newly invented ride.
- Fix: [#5489] Sprite index crash for car view on car ride.
- Fix: [#5730] Unable to uncheck 'No money' in the Scenario Editor.
- Fix: [#5750] Game freezes when ride queue linked list is corrupted.
- Fix: [#5819] Vertical multi-dimension coaster tunnels drawn incorrectly
- Fix: Non-invented vehicles can be used via track designs in select-by-track-type mode.
- Fix: Track components added by OpenRCT2 are now usable in older scenarios.
- Technical: [#5047] Add ride ratings tests
- Technical: [#5458] Begin offering headless build with reduced compile- and run-time dependencies
- Technical: [#5755] Title sequence wait periods use milliseconds
- Technical: Fix many desync sources
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