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

[WIP] Get UPS conversion mod working again after battery overhaul #29983

Closed

Conversation

esotericist
Copy link
Contributor

@esotericist esotericist commented Apr 27, 2019

Summary

SUMMARY: Bugfixes "Get UPS conversion mod working again after battery overhaul"

Purpose of change

With #29651, UPS conversion mods no longer function due to them being reliant on the implicit capacity of the modified device. Since all devices that used to use generic battery charge now rely on magazines for their power storage, they all have an implicit capacity of 0, rendering the UPS conversion mod inoperable.

Fixes #29961

Describe the solution

Leveraging the existence of the capacity_multiplier flag that gets used in the atomic battery toolmod and the double plutonium core toolmod, it's possible to report a modified capacity based on the capacity of the default magazine.

I have for the time being set the capacity_multiplier to 0.8, under the assumption that 80% of the default magazine for any battery-using tool should be adequate to function, as well as providing an additional tradeoff for the UPS conversion mod (less capacity, more convenient charging), since our batteries now are rechargeable in the vehicle mounted recharging station. This value is hardly set in stone, but it must be a value other than 1.0 for my current logic to work.

Describe alternatives you've considered

In #29976 I originally attempted to simply remove the UPS conversion mod, believing it was obsolete, but between comments on that issue and elsewhere and additional problems found in the code, I was convinced this is not yet a correct course of action.

I also considered simply keying the check in item.cpp to whether or not a UPS mod was installed, but I felt that if there's a capacity_multiplier but no capacity clearly something is expected to happen, and this attempts to address that. See also my second point in Additional Context.

Additional context

This is flagged [WIP] for two reasons:

First, I'm not convinced I've implemented the logic right in either item.cpp or player.cpp. While this does work to the best of my ability to determine, it feels a bit on the hackish side, so if there's a better way to do this I'd like to see it. In particular, it's not at all clear to me whether or not use_ups items which are not tools (but which use battery cells) will work correctly, and I ran out of energy shortly before putting up this WIP.

Second, it turns out that the magazines-for-battery implementation broke the Atomic Battery Mod as well, and I feel like fixing that is at least partially connected to this fix (since the problem is interrelated), but I cannot for the life of me figure out how to get the game to accept a plutonium cell as ammo for a tool that has the mod applied. There's something I'm clearly missing, but I don't know what.

Third, I haven't made any attempts to apply this logic to weapons, only held and worn items. It could probably be done, but I wasn't sure if it should be done.

If someone has any ideas, I'd dearly love to hear them (or even better, if someone wants to take this over so they can do it more correctly than I am, that would be extra fantastic).

@Inglonias
Copy link
Contributor

Inglonias commented Apr 27, 2019

It might be feasible just to re-add max_charges to all tools. Does that interact with magazines in an odd way? I thought it would, so I removed it, but if it doesnt it may be a quick fix

@ifreund ifreund added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Items / Item Actions / Item Qualities Items and how they work and interact Items: Battery / UPS Electric power management labels Apr 27, 2019
@esotericist
Copy link
Contributor Author

It might be feasible just to re-add max_charges to all tools. Does that interact with magazines in an odd way? I thought it would, so I removed it, but if it doesnt it may be a quick fix

The code is unfortunately wildly inconsistent in its behavior if an item has both the capacity to hold magazines with battery and a max_charges. This is something that probably needs a lot of love in the long run, but in the short run using max_charges would basically obviate all of your work so far (except when it doesn't).

@drhead
Copy link
Contributor

drhead commented Apr 28, 2019

It may be worth considering a positive multiplier for capacity. From my experiences with the battery system, being able to swap out rechargeable batteries instantly is much more convenient than having a rechargeable tool, since I can use it again instantly when swapping the battery, and I don't have to carry around a UPS -- given how much volume a UPS takes up, you'll almost always be better off carrying spare batteries (2.5L of battery cells will get you about 3300 power, whereas a UPS will have at most 1250). The only use case I would see for it with a negative multiplier would be for tools that live on a recharging plate, or for people with the UPS CBM and reliable bionic power.

Alternatively, the UPS mod could just let the tool be recharged directly while continuing to allow you to swap batteries, with the tradeoff being that you can't install a different battery mod in the tool. Given that the entire point of the UPS mod (renewable energy for tools) is standard to all batteries, the UPS mod really just won't be worth using if it is the same as it was.

@esotericist
Copy link
Contributor Author

It may be worth considering a positive multiplier for capacity. From my experiences with the battery system, being able to swap out rechargeable batteries instantly is much more convenient than having a rechargeable tool, since I can use it again instantly when swapping the battery, and I don't have to carry around a UPS -- given how much volume a UPS takes up, you'll almost always be better off carrying spare batteries (2.5L of battery cells will get you about 3300 power, whereas a UPS will have at most 1250). The only use case I would see for it with a negative multiplier would be for tools that live on a recharging plate, or for people with the UPS CBM and reliable bionic power.

Alternatively, the UPS mod could just let the tool be recharged directly while continuing to allow you to swap batteries, with the tradeoff being that you can't install a different battery mod in the tool. Given that the entire point of the UPS mod (renewable energy for tools) is standard to all batteries, the UPS mod really just won't be worth using if it is the same as it was.

It has been exhaustively demonstrated to me that the assumption of "the entire point of the UPS mod (renewable energy for tools) is standard to all batteries" is not well founded, as there is nuance to the usage of the UPS and powered tools beyond the simple convenience of rechargable batteries.

Based in part on a conversation with @mlangsdorf I'm now reasonably convinced the primary win for the ups conversion mod in the current design of the game is things you aren't going to fully deplete very often, but may very well want to always have a charge. Night vision goggles, radio, geiger counter, etc. For larger things, you really are much better off carrying replacement cells. The UPS conversion mod isn't ever going to be a win for those, and I'm not terribly interested in trying to make it so.

But by the same token, the ups conversion mod shouldn't be a flat out upgrade for the smaller things. Hence the capacity tradeoff. The device works off of UPS transmission, but it can't be used for as long in the absence of a UPS transmitter. I believe this fits the intent of the device, as well as the primary remaining use cases of the UPS system.

@RustyBrute
Copy link

Ups conversion mod had its own capacity because of the old battery system I believe. Now, as the batteries are rechargable and the old batery system is dead, there is no reason for ups to have they own batery capacity (max charge) in my opinion. It should just add the ability to put the tool into the UPS recharge station and charge the batery/magazine in it without removing it. (like adding a connector and external power source dependent charger into the tool).

So you can have your spare batteries with you and still be able to just drop the tool into UPS recharbe station.

@Inglonias
Copy link
Contributor

Inglonias commented Apr 29, 2019

As far as I can tell, that's how the UPS conversion presently works. You can use a UPS item as an external battery for any tool that has this tool mod installed. The problem is that now you HAVE to do that, whereas previously you could just recharge it from the UPS, take it away, and still use it. That's less of a problem because you can charge the batteries in a recharging station now instead and load them, but it also renders the UPS conversion mod as something that's more of a nerf than a benefit, since carrying spare batteries around is easier than carrying an entire UPS around.

The question here is more one of design than a bug fix. Simply put, the new battery system renders the old behavior of the UPS conversion redundant. You can just drop the batteries into a UPS recharging station vehicle part and recharge those. Before, you'd drop the whole tool into the recharger. So why would anyone want to mod their tools like this after the end of the world, when wall outlets don't work anymore?

@Inglonias
Copy link
Contributor

Onto the PR itself, it looks like this will indeed restore something resembling the original behavior of the UPS toolmod, but in the longer term, I think we need to rethink how this toolmod should work, or indeed whether we need it at all.

@esotericist
Copy link
Contributor Author

@RustyBrute The UPS conversion mod never had any effect whatsoever on the capacity of a tool, it just prevented the tool from having charge loaded into it from loose batteries, and the capacity of that tool was whatever its normal capacity was without the mod.

Further, you seem confused about what's actually being being fixed: We already can put tools with battery cells in them in the recharging station. That's not the problem. The problem is the USE_UPS tag is supposed to passively charge tools that have it (such as if you put the UPS conversion mod on your night vision goggles) when you have a UPS device in your inventory (or the CBM).

Now that tools have no max_charge (and instead are reliant on their magazine for capacity), the UPS conversion mod has no actual capacity to fill without going through hoops of some sort like my rough draft here.

@Inglonias As far as I can tell the UPS conversion mod is more or less completely nonfunctional right now, according to the tests and code traces I've done. Even though in theory a modded tool should attempt to use power from a powered UPS in your inventory, it will not even attempt to do so due to the max_charges problem. That's part of why I was tinkering with ammo_capacity() as a workaround.

Also you absolutely do not need the UPS conversion mod to charge a tool with a cell in it. That Just Works already, you don't actually have to unload the tool first.

Onto the PR itself, it looks like this will indeed restore something resembling the original behavior of the UPS toolmod, but in the longer term, I think we need to rethink how this toolmod should work, or indeed whether we need it at all.

Thinking about what exactly this should be doing is part of why I haven't done more with this (or updated it to reflect the changes I made getting vehicle-rig based imaginary tools working).

The whole system is super clunky, and kevin has made noise about wanting to remove the UPS system in general, but mlangsdorf (and others) have argued there's still value in it for smaller devices, and I agree this is the case.

What this means, though, i'm pretty uncertain on.

@esotericist
Copy link
Contributor Author

The question here is more one of design than a bug fix. Simply put, the new battery system renders the old behavior of the UPS conversion redundant. You can just drop the batteries into a UPS recharging station vehicle part and recharge those. Before, you'd drop the whole tool into the recharger. So why would anyone want to mod their tools like this after the end of the world, when wall outlets don't work anymore?

Separately quoting this, because this line of thought is why my original PR was removing the UPS conversion mod. But the point of the UPS conversion mod for many people was not "able to use the recharge station" but instead, as I mention above, the trickle-charge effect in the field for key things you'd want to have power always but don't want to carry reloads around. That's the primary slot this mod fits in the current meta, as I mention in one of my previous posts in this PR.

@esotericist
Copy link
Contributor Author

Apologies, I seem to be repeating myself a bunch, but I'm having trouble sorting my thoughts in a linear fashion.

@esotericist
Copy link
Contributor Author

esotericist commented Apr 29, 2019

After conversing with some others in discord (primarily but not exclusively kevin and mlangsdorf), the expected result for the UPS system is:

  • Devices which use UPS (have the USE_UPS flag, either implicitly or via the UPS Conversion Mod) should have NO power storage themselves
  • The 'current charge' of a UPS-enabled device should be essentially the current charge of the UPS source device
  • Whenever a UPS-enabled device wants to consume power, it should be directly subtracted from its UPS source device
  • Since UPS-compatibility is intended to be in lieu of storage in the device, this means a UPS-enabled device will be non-functional in the absence of a UPS source device which contains charge.

Consequently:

  • All interactions with a device for the purposes of charge should be done using the existing ammo_whatever functions in item.cpp, so those functions should be extended to understand UPS
  • Primary logic for locating available/active UPS source devices and prioritizing their usage should exist in player.cpp
  • The remaining attempts at UPS logic in both game.cpp and the crafting subsystems should, in theory, disappear once everything is moved to use the existing item class methods

Tangentially:

  • The vehicle-mounted UPS-Compatible Recharge Station is completely disconnected from all of this; it's checking for the RECHARGE flag and doesn't care if a device is actually UPS-enabled or not. This might need an updated name/description to better fit its actual function?
  • The UPS Smartphone is a weird artifact that has very little practical purpose, especially in light in the shift in design. Discord discussion suggests it should probably go away as a thing (since it doesn't really bring anything noteworthy to the game itself).

This is a bit of a bigger project than what I originally set out to do with this PR, but there's now a coherent plan to aim for which clarifies the purpose of the conversion mod and should ideally result in a cleaner game implementation

@drhead
Copy link
Contributor

drhead commented Apr 30, 2019

After conversing with some others in discord (primarily but not exclusively kevin and mlangsdorf), the expected result for the UPS system is:

  • Devices which use UPS (have the USE_UPS flag, either implicitly or via the UPS Conversion Mod) should have NO power storage themselves
  • The 'current charge' of a UPS-enabled device should be essentially the current charge of the UPS source device
  • Whenever a UPS-enabled device wants to consume power, it should be directly subtracted from its UPS source device
  • Since UPS-compatibility is intended to be in lieu of storage in the device, this means a UPS-enabled device will be non-functional in the absence of a UPS source device which contains charge.

I like this a lot, this seems a lot more true to the idea of a "unified" power system, while the current system seems more like a play on words with the other, real kind of UPS, and feels like you might as well be hauling one around. One thing I would add, partly for balance and partly for realism, is that tools should probably have reduced volume when they have a UPS conversion mod installed, if that doesn't automatically happen from all tools not having a magazine -- this would likely already happen because tools do seem to inherit the volume of their battery, but I feel that that needs tweaking since a) currently this just adds volume to the tool, which for example makes the UPS now have a ridiculous volume of 3.25L and b) it's possible you would want it to only happen on tools using medium/large batteries, since I wouldn't expect something using a watch battery to get much benefit from switching to exclusively wireless power.

Since this is a significant change to the UPS design, I think the UPS itself should be on the table for a balance pass. Even before the battery changes I'd say it was in a somewhat bad place as far as balance goes on volume -- with small storage batteries having about 2.5x the power for the same volume, the cases where the UPS can really be said to be the best option in the field were somewhat limited (excluding the use of UPS-powered weapons or power armor where it is the only option, which I believe has greatly influenced its current position in balance), but the UPS mod still had lots of utility for just tools because of the convenience and renewability factors. It certainly did not cover the new proposed use case, and if that will be one of its use cases it should be reflected in the volume. Given the size of the batteries being 0.75L (twice as big as the medium ones that are like modern cordless tool batteries), I'd think of the UPS's electronics being more in the range of 0.25-0.75L, so 1-1.5L total. I like to carry about 10 high capacity light batteries, plus the contents of my tools, which is 1.13L of batteries. The UPS would offer less total capacity and would be slightly higher volume, but I think the convenience factor would actually be rather competitive at that point. I'd also say the advanced UPS might need some consideration as it is also probably the second most questionable use of rare, valuable, and non-renewable plutonium cells behind constructing laser sights with them.

An alternative for the UPS might be to shamelessly steal an idea from Aftershock, and introduce a new, wearable mini-UPS that takes a medium battery, either on the Leg (strapped) slot or the Torso (waist) slot. That seems like a good way to cover these cases where you want a trickle charge for tools like NVG and geiger counters without the tradeoff being either carrying around a 3.25L monster or finding a specific bionic and a reasonable source of bionic power.

A summary, since upon revisiting this comment it is clear it is a wall of text:

  • Unmodded tools will be... well, standard. You will be able to swap batteries, you will be able to recharge those batteries or recharge the tools directly. This will likely cover most needs for a good portion of the game. You will have your tool, you can use it with minimal hassle.

  • Larger battery mods and the storage battery mod will obviously make the tool heavier and bulkier, but if you need a lot of power for a specific tool, particularly one you wield or wear full time where the downsides will be lesser (there should absolutely be an encumberance and moves per attack penalty for things with battery mods though, not even for realism but for lack of absurdism), it should be a option that is very much worth considering.

  • The UPS conversion should be for any case where messing with replacement batteries are inconvenient, and with the Unified Power System CBM will likely be the endgame option as well. It will have the upside of making converted tools lighter, in exchange for having to carry or wear a device to power them -- this will favor its use where you have many tools you want to power. This gives the benefit of having them all share a pool of energy, with the upside of using what you want when you want, and the obvious downside that once your power is gone, it is gone from everything.

@HKMist
Copy link

HKMist commented Apr 30, 2019

Happy to see you have new idea on how to manage these problem.
Something i find out these few day on how cdda handle tool, it seem the battery magazine need a lot of follow up work on the base code of cdda.

  1. Process on installing Tool mod don't assume the tool have magazine, so if a tool have loaded magazine it will not show in the "choose which tool to install the mod" menu. This happened on all tool mod.

  2. use_UPS flag added on a tool act only on the tool(and have its own code to handle the power consume), it will not work on the the magazine it installed(code handle is differ when a magazine is present). And what the use_UPS flag do is let UPS/bionic power to recharge the tool(not its magazine) so the UPS mod fail to work.

And a few small bug,

Recharge flag on UPS stop it to unload its installed battery magazine, although you can reload the UPS with another magazine. (And in my opinion i don't think reload a UPS (either with battery magazine/ old battery cell) is a good idea, let is remind its old position of a large power reserve without the need of replace battery on mission(two 2000 battery UPS is more than enough on most situation) but need to recharge/replace before each use seem more fit with the new battery system)

Cellphone have not remove its max charge, so if a cellphone magazine is unload you can turn its on. And in case the code of cdda on display item information, it fail to display how many charge is left on the phone since it is prior to show its magazine information.

@esotericist
Copy link
Contributor Author

@HKMist please report additional bugs in an actual issue report. This PR is specifically about getting the UPS conversion mod to work, any other concerns related to the battery system are not pertinent.

@Inglonias
Copy link
Contributor

I agree with drhead. The big advantage to UPS modded tools can be the complete removal of the battery compartment, resulting in a lighter and smaller tool, but it also means you need to carry an external power source with you.

@esotericist
Copy link
Contributor Author

@drhead @Inglonias While I have thoughts on game balance and content implications on this space, the task here is to get the thing working from a code perspective as I have been told it should work by the devs I spoke with (including the project lead). The other stuff is not pertinent to that end, and this is not the best place to discuss it.

@KurzedMetal
Copy link
Contributor

KurzedMetal commented May 17, 2019

After conversing with some others in discord (primarily but not exclusively kevin and mlangsdorf), the expected result for the UPS system is...

If the new behavior will be having no battery capacity on UPS moded items, can I suggest tweaking "UPS" item to have about ~33% lower volume/mass to make it easier to carry more of them or make it have a battery capacity multiplier (like x1.5, meaning that a heavy battery cell would provide 1.5k)? Or maybe letting me load it with two Batteries instead of one?

Why I am suggesting this? Because of the future removal of the innate capacity of tools. That capacity used to add up and help in the previous implementation, and after the proposed changes we won't have it anymore.

Example: I used recently an UPS modded Heavy Duty Flashlight, MP3, Thermal Electric Suit and Jackhammer to explore an Ice Lab with one capped UPS (1k capacity). Before the suggested change I'd have available about 150+100+150+500 capacity from the items + 1k from the UPS. After this change I'd just have all items starting to drain from the 1k immediately, probably forcing me to return or recharge too often, or carry 2-3 of them instead of just 1 UPS, which is a bit prohibitive on mass/volume (with current values).

@mlangsdorf
Copy link
Contributor

The UPS is not some magic widget that makes your batteries more energy dense. It's a solution for the situation where you have 10 items that are each going to draw 0-200 charges on a mission, so instead of carrying 10 medium cells and bringing them all back with 80% charge, you bring a UPS with two medium cells and discharge them both.

I don't think CDDA supports 2 magazines in the same well so I think you'll have to lug the second cell in inventory, not in the UPS. It's still a weight savings even if it isn't a volume savings.

@KurzedMetal
Copy link
Contributor

You are right, I forgot that now we can swap out Battery Cells from UPS too. So we could bring multiple Battery Cells but a single UPS, while before we would've needed multiple UPS.

That should definitely help on the potential weight/volume issues.

@Dekker3D
Copy link
Contributor

I'm going to suggest something silly here. Applying a UPS mod to a battery. I don't know how common this is in larger batteries, but I know there's AA and 9V batteries that can be charged via USB and can even deliver power while being charged, despite being pretty much empty themselves. Beside that, this seems within the capabilities of someone with 6-8 electronics skill, to me. It should be about the same difficulty as making the recharging station, which you can craft at skill 5-7 depending on the book you use.

So you mod a battery-magazine with the UPS mod, and it'll either drain itself first before drawing from the UPS (good for bringing into a lab) or drain the UPS before draining itself (easy recharge). Maybe these two modes could be a setting on the UPS itself: "drain UPS before tools" or "drain tools batteries before UPS".

@ZhilkinSerg
Copy link
Contributor

I'll close as Stalled, but feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Items: Battery / UPS Electric power management Items / Item Actions / Item Qualities Items and how they work and interact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UPS conversion mod don't work
10 participants