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

SAGA: Deduplicate kScriptTimeTicksPerSecond constant #1183

Merged
merged 1 commit into from Aug 3, 2018

Conversation

Projects
None yet
3 participants
@bonki
Member

bonki commented May 6, 2018

The underlying type of an enum cannot be a float so the following doesn't really make sense:

enum ScriptTimings {
    kScriptTimeTicksPerSecond = (728L/10L),
    kScriptTimeTicksPerSecondIHNM = 72,
    kRepeatSpeedTicks = (728L/10L)/3,
    [...]
};

Because of that, kScriptTimeTicksPerSecond gets truncated to 72 and is therefore identical to kScriptTimeTicksPerSecondIHNM.

This commit removes kScriptTimeTicksPerSecondIHNM, but if the engine actually requires a higher precision then this is not correct and the constant(s) need(s) to be made floating point.

If it's OK to truncate I suggest adding a comment to the enum.

@dafioram

This comment has been minimized.

Show comment
Hide comment
@dafioram

dafioram May 19, 2018

Contributor

It was changed to an enum in commit ea3b0d1. Before that commit (ad0b8f1) it was 72.8 ticks per second, at least thats what the comment said.

At that time the timing stuff was handled in Script::SF_sleep in source/saga/sfuncs.cpp

Contributor

dafioram commented May 19, 2018

It was changed to an enum in commit ea3b0d1. Before that commit (ad0b8f1) it was 72.8 ticks per second, at least thats what the comment said.

At that time the timing stuff was handled in Script::SF_sleep in source/saga/sfuncs.cpp

@dafioram

This comment has been minimized.

Show comment
Hide comment
@dafioram

dafioram Jul 7, 2018

Contributor

I played Inherit the earth a bit with those enums set as floats as well as TicksToMSec to take in a float and operate internally with floats. It played the same as before the changes.

Contributor

dafioram commented Jul 7, 2018

I played Inherit the earth a bit with those enums set as floats as well as TicksToMSec to take in a float and operate internally with floats. It played the same as before the changes.

@sev-

This comment has been minimized.

Show comment
Hide comment
@sev-

sev- Aug 3, 2018

Member

Ok, merging.

Member

sev- commented Aug 3, 2018

Ok, merging.

@sev- sev- merged commit a4ab2d8 into scummvm:master Aug 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment