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

Move PlayerExperience from Infiltrates to InfiltrateFor #20106

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented Jul 5, 2022

This allows customisation per InfiltrateFor trait instead of only for the unit infiltrating. This will allow us to determine what infiltration is worth more than the other.

The reward currently is 10 per infiltration I've given a different reward for infiltrating for cash, 5 + 1% for cash stolen
I've also removed XP from infiltrating fakes

@PunkPun PunkPun force-pushed the fix-infiltrates-gains branch 7 times, most recently from 788244d to e9e0fee Compare July 6, 2022 19:12
@PunkPun PunkPun force-pushed the fix-infiltrates-gains branch 2 times, most recently from 1cbc9e2 to 6624be8 Compare July 7, 2022 07:36
@AspectInteractive2
Copy link
Contributor

AspectInteractive2 commented Jul 31, 2022

I think the experience added by infiltration is bugged, as it reset the experience to 0 initially, and then it sets it to 2635 somehow as shown below - there doesn't seem to be a pattern to it. I've tested the original game's experience and normally it increments by 50 per infiltration as you've described in your original post.

image

@PunkPun
Copy link
Member Author

PunkPun commented Jul 31, 2022

I think the experience added by infiltration is bugged, as it reset the experience to 0 initially, and then it sets it to 2635 somehow as shown below - there doesn't seem to be a pattern to it. I've tested the original game's experience and normally it increments by 50 per infiltration as you've described in your original post.

image

It seems I have forgotten to divide InfiltratesForCash percentage by 100. The 2635 was probably meant to be 18.
I additionally nerfed this gain to 5 + 1% and removed XP from infiltrating fakes

@PunkPun PunkPun force-pushed the fix-infiltrates-gains branch 2 times, most recently from 6fef14b to 0fbb5a0 Compare July 31, 2022 19:02
Mailaender
Mailaender previously approved these changes Aug 6, 2022
@PunkPun
Copy link
Member Author

PunkPun commented Aug 19, 2022

rebased

@Mailaender
Copy link
Member

Needs a small rebase.

@PunkPun PunkPun mentioned this pull request Oct 31, 2022
@PunkPun PunkPun force-pushed the fix-infiltrates-gains branch 2 times, most recently from 4193e5a to 8782284 Compare November 16, 2022 19:24
@anvilvapre
Copy link
Contributor

Looking at the code for the first time it seems that the InfiltrateFor classes share a lot of the same code. And that this pull request even add more duplicate code to those classes. I.e. you add the same line of code to many files.

There is no way to refactor these classes, let them inherit or make use of a util class?

@PunkPun
Copy link
Member Author

PunkPun commented May 30, 2023

Looking at the code for the first time it seems that the InfiltrateFor classes share a lot of the same code. And that this pull request even add more duplicate code to those classes. I.e. you add the same line of code to many files.

There is no way to refactor these classes, let them inherit or make use of a util class?

I don't think it's necessary, these are fairly tiny traits. A util class wouldn't help much with code sharing as there's actually very little duplicated code here.

@Mailaender
Copy link
Member

Needs a rebase.

@PunkPun PunkPun force-pushed the fix-infiltrates-gains branch 2 times, most recently from 16064d7 to 5f4ad28 Compare July 25, 2023 18:19
@PunkPun
Copy link
Member Author

PunkPun commented Jul 25, 2023

Rebased and update rule fixed

@Mailaender Mailaender merged commit 4cd4e1f into OpenRA:bleed Jul 25, 2023
3 checks passed
@PunkPun PunkPun deleted the fix-infiltrates-gains branch July 25, 2023 19:17
@PunkPun
Copy link
Member Author

PunkPun commented Jul 25, 2023

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants