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.

Sign up for GitHub

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

Jump to bottom

Replace last uses of 'seconds' with ticks in world simulation #19265

Merged
merged 2 commits into from
Apr 19, 2021
Merged

Replace last uses of 'seconds' with ticks in world simulation #19265

merged 2 commits into from
Apr 19, 2021

Conversation

reaperrr
Copy link
Contributor

This was long overdue.

@pchote
Copy link
Member

pchote commented Mar 21, 2021 •
edited
Loading

I'm wondering if it would make more sense to keep the announcement intervals in seconds and change the code to use real seconds rather than ticks - see #18401.

@reaperrr
Copy link
Contributor Author

Probably makes sense... though we have a few similar places that already use ticks, so we'd have to convert those to real seconds as well if we want to be consistent about it.

@reaperrr reaperrr force-pushed the bye-magic-25 branch 2 times, most recently from 5e999e4 to 06dc257 Compare March 28, 2021 13:13
@reaperrr
Copy link
Contributor Author

Probably makes sense... though we have a few similar places that already use ticks, so we'd have to convert those to real seconds as well if we want to be consistent about it.

I will handle that in a separate PR, so removed the announcement/notifier changes from here.

Also added rename for ScaredyCat.PanicLength to PanicDuration, part for consistency and part to find/remember it more easily when we go for the lower default tick rate.

@pchote
Copy link
Member

pchote commented Mar 28, 2021 •
edited
Loading

On that note, shouldn't we rename the crate Lifetime to Duration too?

@reaperrr reaperrr mentioned this pull request Mar 28, 2021
Merged
@reaperrr
Copy link
Contributor Author

reaperrr commented Apr 1, 2021

On that note, shouldn't we rename the crate Lifetime to Duration too?

Done. Haven't tested the updated update rule, though.

pchote
pchote reviewed Apr 2, 2021
View reviewed changes
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

One small nit otherwise LGTM.

I have tested that the updated update rule behaves correctly.

OpenRA.Mods.Common/UpdateRules/Rules/20201213/ReplaceCrateSecondsWithTicks.cs Outdated Show resolved Hide resolved
@reaperrr
Copy link
Contributor Author

reaperrr commented Apr 3, 2021

Updated.

pchote
pchote previously approved these changes Apr 3, 2021
View reviewed changes
@reaperrr
Copy link
Contributor Author

Rebased.

@reaperrr
Replace * 25 in internal tick defaults with actual total
b640b32 
While the idea behind it is understandable,
this was inconsistent with the bulk of other defaults.
@reaperrr
Change Crate.Lifetime from 'seconds' to ticks
13d490c 
As far as I could tell, this was the last place that still
used 'seconds' instead of ticks, apart from
some sound notification intervals (which are better
converted to real [milli]seconds).

Also renamed ScaredyCat.PanicLength to PanicDuration for
consistency and easier finding.
@teinarss teinarss merged commit 53e6d97 into OpenRA:bleed Apr 19, 2021
@teinarss
Copy link
Contributor

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/f27b819ddcc91bc65aa77637535eb8b4e619d26a...1e4f5783affbb5b8101763456c9f654a30cbdc29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Smittytron Smittytron Smittytron left review comments

@teinarss teinarss teinarss approved these changes

@pchote pchote pchote left review comments

Assignees
No one assigned
Labels
PR: Needs +2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@reaperrr @pchote @teinarss @Smittytron