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

Use real (milli)seconds for intervalled notifications #19295

Merged
merged 1 commit into from Apr 21, 2021
Merged

Use real (milli)seconds for intervalled notifications #19295

merged 1 commit into from Apr 21, 2021

Conversation

reaperrr
Copy link
Contributor

Instead of using fake (aka gamespeed/tick-based) seconds.

First commit is identical to #19265, but whichever PR gets merged second will need a rebase anyway due to their update rules.

@pchote
Copy link
Member

pchote commented Apr 2, 2021 •
edited

Can we please also update ResourceStorageWarning and PowerManager to complete (i think) the set?

This might also be a good opportunity to also add an interval to ActorLostNotification to fix the issues with super loud "UNIT LOST" on surrender/loss and also close #3449?

@reaperrr
Copy link
Contributor Author

reaperrr commented Apr 2, 2021

This might also be a good opportunity to also add an interval to ActorLostNotification to fix the issues with super loud "UNIT LOST" on surrender/loss and also close #3449?

Generally I agree we should do that, but I'd like to leave that for a follow-up (or at least a separate PR, since it doesn't really depend on this one).
We need to track that in a central place, so it might require some more work and thinking than the places we touch here. We should also first consider what other places that also don't have any notification rate limits might want something similar (ProductionQueue notifications come to mind).

@pchote
Copy link
Member

pchote commented Apr 2, 2021

Yup, makes sense. Please do fix the other two here though!

@reaperrr
Copy link
Contributor Author

reaperrr commented Apr 2, 2021

Yup, makes sense. Please do fix the other two here though!

Done. Also found another place (PlayerResources.InsufficientFundsNotificationDelay).

pchote
pchote reviewed Apr 8, 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.

A few small and easily fixable issues. My testcase was to start a game, quickly use /all and /lowpower, and then place a tech center before the 10s power advice interval has elapsed.

OpenRA.Mods.Common/Traits/Player/BaseAttackNotifier.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Player/HarvesterAttackNotifier.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Player/PlayerResources.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Player/ResourceStorageWarning.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Sound/AnnounceOnKill.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Power/Player/PowerManager.cs Outdated Show resolved Hide resolved
@reaperrr
Copy link
Contributor Author

reaperrr commented Apr 9, 2021

Updated.

pchote
pchote previously approved these changes Apr 10, 2021
View reviewed changes
@Smittytron
Copy link
Member

Needs rebase.

@reaperrr
Use real (milli)seconds for notifications
e480506 
Allows for more fine-grained control, while
adding independence from gamespeed and getting
rid of magic * 25 multiplications.
@reaperrr
Copy link
Contributor Author

Rebased.

@teinarss teinarss merged commit f1a9a51 into OpenRA:bleed Apr 21, 2021
@teinarss
Copy link
Contributor

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/d7cf9477eba791a2cc37e1041b5a471189d7637e...8bb78d53789a4bb348970de2d7a2f895ff25ea46

@reaperrr reaperrr deleted the bye-magic-25-pt2 branch July 2, 2021 13:24
dragunoff added a commit to dragunoff/OpenRA that referenced this pull request Dec 13, 2022
@dragunoff
Fix error in PlayerResources trait documentation
9aa361e 
This was missed in OpenRA#19295
@dragunoff dragunoff mentioned this pull request Dec 13, 2022
Merged
PunkPun pushed a commit that referenced this pull request Dec 16, 2022
@dragunoff @PunkPun
Fix error in PlayerResources trait documentation
8513de0 
This was missed in #19295
Zero-Fanker pushed a commit to OpenRA-CN/OpenRA that referenced this pull request Mar 6, 2023
@dragunoff @dnqbob
Fix error in PlayerResources trait documentation
8d3f4c5 
This was missed in OpenRA#19295
thumperward pushed a commit to thumperward/OpenRA that referenced this pull request Oct 9, 2023
@dragunoff @PunkPun
Fix error in PlayerResources trait documentation
b19bf3b 
This was missed in OpenRA#19295
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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 @Smittytron @teinarss