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

[CR] Make Power Storage use chest slots #33812

Closed
wants to merge 25 commits into from

Conversation

dunstad
Copy link
Contributor

@dunstad dunstad commented Sep 5, 2019

Summary

SUMMARY: Interface "Power storage can be uninstalled, uses slots"

Purpose of change

This makes the power storage bionics take up 1 slot when bionic slots is enabled, and changes the player display menu and (soon) the bionics menu to display bionics you have multiples of more compactly.

Describe the solution

Power storage bionics now take up slots in the torso, and are uninstallable. A flag for bionics that may be installed more than once is added, and the uninstall procedure for bionics is modified to support multiple bionics with the same ID.

If you have two power storage bionics installed, the @ menu will now display them as "2 Power Storage" . The bionics menu now also displays them this way.

Describe alternatives you've considered

We could not modify the UI code and have it just print Power Storage over and over, but that seems messy.

The Power Storage bionics could be adjusted to take up more slots if that's necessary.

Additional context

There are 80 slots in the torso, so at the current 1 slot value, if you filled all of those with Power Storage Mk. 1, you'd have 8000 power. If you filled them all with Mk. 2, you'd have 20,000 power.

2_power_storage

uninstall

bionics_menu

@Night-Pryanik
Copy link
Contributor

I'd prefer not installing it "somewhere in the torso", but rather in some bionic specifically designed to contain power storage batteries.

@ZhilkinSerg ZhilkinSerg added [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Bionics CBM (Compact Bionic Modules) Info / User Interface Game - player communication, menus, etc. labels Sep 5, 2019
@Fris0uman
Copy link
Contributor

What happens when you have multiple bionic with same id and try to remove one, what's displayed in the uninstall menu?

@dunstad
Copy link
Contributor Author

dunstad commented Sep 5, 2019

Only one of them is uninstalled, and the menu looks the same as now.

Edit: I should clarify, it seems like the uninstall menu already showed multiple bionics by stacking them with numbers without me having to make any changes there. I just realized that there wouldn't be any way to see this normally, so I'll add a screenshot in the near future.

@SirPendrak
Copy link
Contributor

"Describe the solution
Power storage bionics now take up slots in the torso, and are uninstallable. "

Why is that? If you install too many power storages and then you relize you have not enough place for your favourite bionics youre doomed to be walking energy storage without use for eternity? And it makes it impossible to upgrade from MKI's to MKII's in lategame.

@tinukedaya
Copy link
Contributor

tinukedaya commented Sep 5, 2019

"Describe the solution
Power storage bionics now take up slots in the torso, and are uninstallable. "

Why is that? If you install too many power storages and then you relize you have not enough place for your favourite bionics youre doomed to be walking energy storage without use for eternity? And it makes it impossible to upgrade from MKI's to MKII's in lategame.

I guess you misunderstood. It means that you CAN indeed uninstall them. So yes, you can free up slots later or upgrade to mkII's.

@dunstad dunstad changed the title [WIP] [CR] Make Power Storage use chest slots [CR] Make Power Storage use chest slots Sep 6, 2019
@kholat
Copy link
Contributor

kholat commented Sep 11, 2019

Torso should have more slots then imo.

@Amoebka
Copy link
Contributor

Amoebka commented Sep 12, 2019

I would say the amount of torso slots is adequate, but too many bionics compete for them. Almost all bionics take at least some torso slots, sometimes for very arbitrary reasons, making it the bottleneck.

20.000 max charge also seems way too generous, even though in practice it will be more like 10.000 because of the aforementioned torso slot competition.

debugmsg( "Tried to install bionic %s that is already installed!", b.c_str() );
return;
}

units::energy pow_up = bionics[b].capacity;
mod_max_power_level( pow_up );
if( b == "bio_power_storage" || b == "bio_power_storage_mkII" ) {
if( pow_up > 0_J ) {
add_msg_if_player( m_good, _( "Increased storage capacity by %i." ),
units::to_kilojoule( pow_up ) );
Copy link
Member

@anothersimulacrum anothersimulacrum Oct 21, 2019

Choose a reason for hiding this comment

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

The will not report if the max power was increased by an amount less than one joule.
Additionally, the message will show zero if the power was increased by any value less than 1 kilojoule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks for letting me know. I'll look at the new menu code to see how they handle power display. I wasn't sure what to use for 0 in the new system.

Copy link
Member

Choose a reason for hiding this comment

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

The message being wrong is technically not your problem to solve (you don't touch it), but it'd be nice if you did fix it.

Copy link
Member

Choose a reason for hiding this comment

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

In fact, I might suggest (if there's no function to format the power like the menu/sidebar do), that you don't do that and then you or someone else makes that function and uses it in the proper places in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 0_J to 0_mJ, it looks like that's the smallest unit.

@stale
Copy link

stale bot commented Nov 23, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Nov 23, 2019
@stale
Copy link

stale bot commented Dec 23, 2019

This issue has been automatically closed due to lack of activity. This does not mean that we do not value the issue. Feel free to request that it be re-opened if you are going to actively work on it

@stale stale bot closed this Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bionics CBM (Compact Bionic Modules) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON stale Closed for lack of activity, but still valid.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants