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

fix: power armor actually benefits from advanced UPS efficiency, adjust efficiency ratio #4842

Merged
merged 8 commits into from
Jun 20, 2024

Conversation

chaosvolt
Copy link
Member

@chaosvolt chaosvolt commented Jun 20, 2024

Checklist

Required

Purpose of change

This fixes a flaw I found with the advanced UPS not gaining any benefit when used with light power armor like it does for other items, and along the way I ended up deciding to make the efficiency multiplier used elsewhere by advanced UPS a little neater.

On further testing it was found that this mainly applies to power draw values that demand an odd number of charges, which is why light power armor was most strongly affected by this. @KheirFerrum provided a proper fix for that that compensates for odd-number charge consumption when using advanced UPS.

Fixes #3765

Describe the solution

  1. In bionics.cpp. converted use of advanced UPS in Character::consume_remote_fuel to consume 0.5x charges when using advanced UPS, instead of a janky 0.6x.
  2. In character.cpp, added a more graceful handling of advanced UPS handling in Character::use_charges, provided by Kheir. Basically, because it's being fed a raw int for how much power it needs to consume from the UPS, it can't gracefully handle cutting the power draw in half in those cases, and that's quite a big difference considering with UPS use each charge is a full kilojoule. Kheir's function checks if the power draw would be an odd number and applies a reduction 50% of the time so that net behavior over a long period of time will match expected results you'd get if power draw was an even number.
  3. In character_turn.cpp, converted relevant multipliers in Character::process_items as well.
  4. In ranged.cpp, set ranged::gunmode_checks_weapon to defined adv_ups_drain as half that of ups_drain
  5. In visitable.cpp, set visitable<inventory>::charges_of and visitable<Character>::charges_of to use new multiplier for advanced UPS.

Describe alternatives you've considered

Banging the code against a rock until every single power-related function and all battery JSON has been converted to properly deal in joules instead of kJ. We'll probably have to suffer than someday anyway if we ever JSONize UPS items...

Testing

  1. Compiled and load-tested.
  2. Spawned in as a power armor soldier.
  3. Tested a full set of light power armor (0.75+0.25 kW, total 1 kW). Power draw after 5 minutes was 297 (average 300) for standard UPS, 161 (average 150) for advanced UPS.
  4. Tested medium power armor minus helmet (1.5 kW). Power draw after 5 minutes was 297 (average 300) for standard UPS, 161 (average 150) for advanced UPS.
  5. Temporarily set heavy power armor's suit to draw 3 kW on its own (normally 2.25 kW) and tested. Power draw after 5 minutes was 900 (average 900) for standard UPS, 444 (average 450) for advanced UPS.
  6. Spawned in a V29 laser pistol, tried firing it without any UPS and confirmed it asks for 10 UPS or 5 advanced UPS charges.
  7. Fired it with advanced UPS in inventory, confirmed it uses 5 charges as expected.
  8. Checked affected files for astyle.

Power draw is weird and random, so exact matches to expected average aren't as important as confirming the difference exists.

Comparison of UPS items after 5 minutes of light power armor+helm use each:
image

Modified 3-kW heavy power armor test results:
image

Laser pistol message:
image

Additional context

At some point I want to unhardcode UPS items outright which will mean a JSONized efficiency rating for each item, which means the current hacks will likely need to be replaced with something that can handle it better. That might even require an outright overhaul of power draw code, use_charges_if_avail, or more to behave more sanely, I don't know.

@github-actions github-actions bot added the src PR changes related to source code. label Jun 20, 2024
@chaosvolt chaosvolt marked this pull request as draft June 20, 2024 19:20
chaosvolt and others added 2 commits June 20, 2024 15:01
Co-Authored-By: KheirFerrum <102964889+KheirFerrum@users.noreply.github.com>
Copy link
Contributor

autofix-ci bot commented Jun 20, 2024

Autofix has formatted code style violation in this PR.

I edit commits locally (e.g: git, github desktop) and want to keep autofix
  1. Run git pull. this will merge the automated commit into your local copy of the PR branch.
  2. Continue working.
I do not want the automated commit
  1. Format your code locally, then commit it.
  2. Run git push --force to force push your branch. This will overwrite the automated commit on remote with your local one.
  3. Continue working.

If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT.

@chaosvolt
Copy link
Member Author

Okay, it should work. By all right it should now ONLY be touching bits of code specific to advanced UPS.

But now all of a sudden the regular UPS is drawing about half as much power as it should be too!?
image

Co-Authored-By: KheirFerrum <102964889+KheirFerrum@users.noreply.github.com>
@chaosvolt
Copy link
Member Author

And thanks to Kheir for figuring out the problems, it seems to work now. Will re-mark as ready after I finish updating the PR OP.

@chaosvolt chaosvolt marked this pull request as ready for review June 20, 2024 21:55
src/character.cpp Outdated Show resolved Hide resolved
chaosvolt and others added 2 commits June 20, 2024 17:07
Co-Authored-By: KheirFerrum <102964889+KheirFerrum@users.noreply.github.com>
@chaosvolt chaosvolt merged commit a31bf10 into cataclysmbnteam:main Jun 20, 2024
13 checks passed
@chaosvolt chaosvolt deleted the advanced-ups-tweak branch June 21, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
src PR changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Advanced UPS' hardcoded power use bonus does not work for power armor
3 participants