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

JSONizing CBM slot feature #27780

Merged
merged 4 commits into from
Jan 26, 2019
Merged

JSONizing CBM slot feature #27780

merged 4 commits into from
Jan 26, 2019

Conversation

Firestorm01X2
Copy link
Contributor

@Firestorm01X2 Firestorm01X2 commented Jan 22, 2019

JSONnizing CBM slot feature

Feaure was hidden with debug mutation. Now it is can be enabled or disabled via EXTERNAl OPTION "CBM_SLOTS_ENABLED". So it can be enabled via mod.

#15937

Feature is still functional but may not be fully balanced. That is why it is disabled by default. But now it can be enabled via JSOn editing. So It can be enabled via mod

Feature is disabled by default. So it is just infrastructure change.

Summary

SUMMARY: Content "JSONnizing CBM slot feature"

Purpose of change

https://github.com/CleverRaven/Cataclysm-DDA/projects/6

JSONnizing CBM slot feature

Describe the solution

Feaure was hidden with debug mutation. Now it is can be enabled or disabled via EXTERNAl OPTION "CBM_SLOTS_ENABLED". So it can be enabled via mod.

Debug mutation to enable slot system was removed. It is not needed anymore.

Describe alternatives you've considered

Additional context

None.

Enable CBM slots
@Firestorm01X2 Firestorm01X2 changed the title [WIP] Enable CBM slots [WIP] Restoring CBM slots Jan 22, 2019
@Night-Pryanik
Copy link
Contributor

#15937 is for reference.

@Firestorm01X2
Copy link
Contributor Author

Firestorm01X2 commented Jan 22, 2019

@Night-Pryanik
Can I soften CBM installation skill requrments here? It is additonal restriction for player. CBM sofware is big TODO and player should be tempted to constatly install and reinstall CBMs now. So it should be easier to balance new restriction.

Good think that quick testing indicated that feature still funtional. Nothing seems to be broken for this years.

Astyling
@Night-Pryanik
Copy link
Contributor

Can I soften CBM installation skill requrments here?

No. It should be done in a separate PR.

Furthermore, please do tell why do you think there is a need for softer requirements. With usual "expected vs. actual" comparison, numbers, calculations etc. The "additional restriction" is not a valid reason for easier installation.

@Firestorm01X2
Copy link
Contributor Author

Furthermore, please do tell why do you think there is a need for softer requirements.

To balance new restriction. Guranteed way to install CBMS are big TODO for now.

With slot system player should constantly tweak himself to fit current needs. It will be hard until raising skills.

@Firestorm01X2 Firestorm01X2 changed the title [WIP] Restoring CBM slots Restoring CBM slots Jan 22, 2019
@Firestorm01X2
Copy link
Contributor Author

No. It should be done in a separate PR.

Then it is ready.

@Night-Pryanik
Copy link
Contributor

To balance new restriction.

In this case CBM slots restriction should raise skill requirements instead. Imagine - now we have limited space in our body, and every new CBM should be installed in a way that shouldn't interfere with the previously installed CBMs. Each new CBM require more and more skill to successful installation.

@Firestorm01X2
Copy link
Contributor Author

Firestorm01X2 commented Jan 22, 2019

In this case CBM slots restriction should raise skill requirements instead.

It may just force player to install only few CBMS instead of filling slots.
Also do not forget that removing CBMs require resources. And now player will have to remove CBMs to free some space.

Something has to compensate requirement to constantly jiggle CBMs.

@Night-Pryanik
Copy link
Contributor

It may just force player to install only few CBMS instead of filling slots.

And why should they necessarily fill slots in the first place?

Something has to compensate requirement to constantly jiggle CBMs.

Bionics are already OP, big advantages without any drawbacks. Difficulties and complications of bionics (un)installation are already compensated by their own coolness.

@Firestorm01X2
Copy link
Contributor Author

Firestorm01X2 commented Jan 22, 2019

And why should they necessarily fill slots in the first place?

To as adapted and strong as player can be.

Bionics are already OP, big advantages without any drawbacks. Difficulties and complications of bionics (un)installation are already compensated by their own coolness.

If certain bionics are OP than certain CBMs istelf should be nerfed in the first place.

@Night-Pryanik
Copy link
Contributor

To as adapted and strong as player can be.

Every wish has its own price. In this case its increased requirements on skills and difficulty of the (un)installation process.

@Firestorm01X2
Copy link
Contributor Author

Firestorm01X2 commented Jan 22, 2019

Every wish has its own price. In this case its increased requirements on skills and difficulty of the (un)installation process.

Slots are restiction iself. I recommend to tweak certain CBM. For example ta make really powerfull CBM fill almost all free space in player body. So player more reason to uninstall it or even do not install it at all.
It is much simplier solution.

@I-am-Erk
Copy link
Member

What has changed to make it "time for this"? I feel like CBM slots are still a bit of a sloppy way to alter CBMs, which currently aren't that direly in need of nerfing. There have been a number of smoother systems suggested in the last while.

Personally I think it will be "time for this" when mutations have been brought into line a little bit more.

@Firestorm01X2
Copy link
Contributor Author

Firestorm01X2 commented Jan 22, 2019

What has changed to make it "time for this"?

Delaying of 0.D. Feature was disabled in the first place because people thouht that 0.D was around corner. It was almost 2 years ago.
Also this feature much less articial in comparsion to other restrictions. And should be good balancing tool. Much better than unexpectedly rare anastetic kits.

@Firestorm01X2
Copy link
Contributor Author

@I-am-Erk
Copy link
Member

The feature was delayed to 0.D because it was only halfway implemented. It is still only halfway implemented, and this is a particularly odd time to cite 'delayed release of 0.D' as a reason, since progress to the next version is moving pretty steadily right now.

Do you intend to complete the other elements of bionic slots, like balancing bionic slot availability and slot occupation, or making faulty bionics a part of the bionic piece itself rather than a separate bionic? Per the project, those should be done before the slots are mainlined. If you want to go through that stuff, then it's not a half-done feature and could certainly be re-enabled. Be aware though that you'll probably face significant discussion and debate around the balance element, because it's a complex feature.

@Firestorm01X2
Copy link
Contributor Author

Firestorm01X2 commented Jan 22, 2019

@I-am-Erk

Do you intend to complete the other elements of bionic slots, like balancing bionic slot availability and slot occupation, or making faulty bionics a part of the bionic piece itself rather than a separate bionic?

You, know. You right. I think I am not going to do that.

Instead let's reporpuse this PR. Now it will be just JSON switch to enable CBM slots.
And it will be disabled by default.

Modders now can enable it.

It still more convinent then debug mutation.

@Firestorm01X2 Firestorm01X2 changed the title Restoring CBM slots JSONnizing CBM slot feature Jan 22, 2019
CBM slots diabled by defualt
@I-am-Erk
Copy link
Member

Making it a json switch is pretty reasonable, having it concealed in debug is a bizarre solution.

@Firestorm01X2
Copy link
Contributor Author

Firestorm01X2 commented Jan 22, 2019

Making it a json switch is pretty reasonable, having it concealed in debug is a bizarre solution.

I think it was done before EXTERNAL OPTIONS were implemented. Likely that is the only reason.

Removing DEBUG_CBM_SLOTS mutation
@Firestorm01X2 Firestorm01X2 changed the title JSONnizing CBM slot feature JSONizing CBM slot feature Jan 22, 2019
@nexusmrsep
Copy link
Contributor

The feature was delayed to 0.D because it was only halfway implemented. It is still only halfway implemented, and this is a particularly odd time to cite 'delayed release of 0.D' as a reason, since progress to the next version is moving pretty steadily right now.

If the other half is described and agreed upon, just point @Firestorm01X2 towards it. Postponing by saying "someone will pick it up after 0.D" sounds like saying "lets not think about it unless we see some pegazi flying in the sky". 0.D is a mythical being, you cannot guarantee seeing it in a month, or a year, and also since when is anything postponed on this ever-rolling train of a development system, when there is someone willing to do the work, and the work itself is agreed on? I believe only @Coolthulhu absence is the reason why it is stuck. It predates autodoc and anesthesia and was meant as a first major limiting factor. It finally addresses the scarcity of human body. (hmm.... threshold for CBMs... no longer human... robocop.... hmmm). So I say - unless there are any significant objections toward the slot concept itself - instead postponing it, its as good time to do it as any other.

@Firestorm01X2
Copy link
Contributor Author

I have to point the fact that feaure shows slots and limits installation.

I don't know what is half implemented there but feature looks functional.

@ZhilkinSerg ZhilkinSerg added [JSON] Changes (can be) made in JSON Bionics CBM (Compact Bionic Modules) labels Jan 22, 2019
@ZhilkinSerg ZhilkinSerg added the [C++] Changes (can be) made in C++. Previously named `Code` label Jan 22, 2019
@I-am-Erk
Copy link
Member

If the other half is described and agreed upon, just point @Firestorm01X2 towards it. Postponing by saying "someone will pick it up after 0.D" sounds like saying "lets not think about it unless we see some pegazi flying in the sky". 0.D is a mythical being, you cannot guarantee seeing it in a month, or a year, and also since when is anything postponed on this ever-rolling train of a development system, when there is someone willing to do the work, and the work itself is agreed on? I believe only @Coolthulhu absence is the reason why it is stuck. It predates autodoc and anesthesia and was meant as a first major limiting factor. It finally addresses the scarcity of human body. (hmm.... threshold for CBMs... no longer human... robocop.... hmmm). So I say - unless there are any significant objections toward the slot concept itself - instead postponing it, its as good time to do it as any other.

Personally I'd agree with the arguments in the project itself: slots alone aren't well balanced, particularly since broken CBMs take up an extra slot for the broken part, and the treatment for broken CBMs is just to remove that extra part. At the very least, the broken CBM system should be fixed so that you get a "leaky power module" or whatever, and have to repair that messed up system rather than getting a strange filler piece. It would make sense to do this first, and that's recommended in the project.

Besides that, CBM slot cost should be examined and balanced in a meaningful way per #16920 . Whether that's done before or after implementing them is debatable; personally I'd say the slots should be implemented once we know they will work appropriately, rather than implementing slots first and then seeing from how vocal the outcry is whether or not they've been done reasonably. What kind of bionic characters are prohibited by the slot system, what kind are permitted? That kind of stuff should be discussed... IMO it should be discussed before the PR phase.

@Firestorm01X2
Copy link
Contributor Author

If only problem is balancing then slots are ready.

Broken CBM occupuing part actually make sense. You have to remove it. Just like it is now. Other things can be done later.

@kevingranade kevingranade merged commit ca3a26e into CleverRaven:master Jan 26, 2019
kevingranade pushed a commit that referenced this pull request Jan 29, 2019
* Replace DEBUG_CBM_SLOTS mutation with an option.
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` [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants