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

Added carryalls to spectator Economy statistics #20566

Merged
merged 1 commit into from Dec 21, 2022
Merged

Added carryalls to spectator Economy statistics #20566

merged 1 commit into from Dec 21, 2022

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented Dec 19, 2022

supersedes #20079

@abcdefg30 abcdefg30 mentioned this pull request Dec 19, 2022
Closed
@PunkPun PunkPun added this to the Next release milestone Dec 19, 2022
@pchote
Copy link
Member

pchote commented Dec 19, 2022

Can we rename UpdatesDerrickCount to UpdatesSpecialActorCount (or UpdatesModActorCount or something else sufficiently generic) and reuse the rest of the derrick ui plumbing instead of hardcoding multiple mod-specific actors here?

@PunkPun
Copy link
Member Author

PunkPun commented Dec 20, 2022

I think here hardcoding is the way to go. Unless a spectator UI refactor is planned I think we should just follow the convention. Using the tag for both will just cause issues in the future.

dragunoff
dragunoff approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@dragunoff dragunoff left a comment

Choose a reason for hiding this comment

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

Works and LGTM 👍

I gotta agree with @PunkPun here - hardcoding seems like the simpler way to go instead of trying to lump dericks and caryalls together. The observer UI is anyways full of CNC-specific idioms and stuff. I don't consider it generic at all.

@Mailaender Mailaender merged commit 55cf40e into OpenRA:bleed Dec 21, 2022
@PunkPun PunkPun deleted the add-caryall-statistics branch December 21, 2022 20:07
@Mailaender
Copy link
Member

bleed
prep

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

@dragunoff dragunoff dragunoff approved these changes

Assignees
No one assigned
Labels
PR: For stable PR: Needs +2
Projects
None yet
Milestone
Release 20230225
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@PunkPun @pchote @Mailaender @dragunoff