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

Resource converter module interface #1256

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

AlchemistCH
Copy link
Contributor

Development for #1255
ConverterFields.cs gives full set of interfaces for ModuleResourceConverter, allowing the scripts not only to turn converter on and off, but also access its performance data. Besides data available to the player in the right-click menu, it also allows to read some converter parameters set in part's config file (including conversion recipe) as well as the actual current resource consumption and production.
ConverterValue part struct gives both a way to address specific converter modules and an option to address all of them by calling the same suffixes on it (for single-module case there won't be difference).
Such double-layer system can be useful for handling other cases, like potentially multiple science modules on the same part.

@AlchemistCH AlchemistCH changed the title Resource converter (prototype) Resource converter module interface Nov 22, 2015
@Dunbaratu
Copy link
Member

@AlchemistCH sorry nobody seems to have noticed this - I'm going through the pending PR's and I see this has gotten no action for quite a while.

In a sense the rest of us have gotten a sort of "battlefield promotion" on the project because erendrake's new job has left him without a lot of free time to do a lot of the managerial stuff he normally does. I don't know the story of this PR or why nothing has been said about it, but we should give it a good look.

Right now we're in the middle of a feature freeze for a 0.19.0 release, as we get all the last things merged in, and we want desperately to get it out before KSP 1.1 drops, so I'd be reluctant to add a new feature normally, but this one looks fairly self-contained in just one module. We'd have to do some updating on it because the mod infrastructure has had a few changes, but it's not that bad to do it.

I'll have to see what the others think.

@hvacengi
Copy link
Member

I think that the main question previously on this issue surrounded the fact that it's using PartValue instead of PartModuleFields. Looking over the code though, it looks as though each individual conversion is a separate part module (i.e. there's one for ore => liquid fuel, another for ore => liquid fuel + oxidizer, another for ore => monoprop, etc.). Given that, it may actually make more sense to keep it as a part, or to find a good way to search for the conversion type information from the raw PartModuleFields. I know that I at least have been trying to keep as many PartModule related functions in the PartModuleFields section, since it kind of follows the compositing model that @erendrake has talked about.

I guess the summary of that is that while I think it's an incredibly useful feature, we really need to think about the architecture of it more carefully.

@Dunbaratu
Copy link
Member

okay lets leave it out of 0.19.0 then

@AlchemistCH
Copy link
Contributor Author

The PartModule-based class may be good as it is (well, with suffix definitions altered according to the latest updates), but the Part-based class here is somewhat more of a tech demo for handling multiple modules of the same kind.
Some of the later concepts include turning this mutiple-module-of-the-kind handler into intermediate class attached to the base Part class and dumping suffixes on it - allowing the part to have multiple classes at once accessible right on the part level (it's possible to rearrange the part system this way without breaking existing scripts), but it's a rater big thing...
So, yeah, better leave it out for now, especially with all the big changes in this update

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

Successfully merging this pull request may close these issues.

3 participants