ShotgunNinja / Kerbalism

Hundreds of Kerbals were killed in the making of this mod.
The Unlicense
43 stars 19 forks source link

Science hijacker #44

Closed DMagic1 closed 7 years ago

DMagic1 commented 7 years ago

A few things about your science dialog hijacker:

It looks like you are just constantly checking to see if the dialog is active from Update. A better way to handle it, unless there is some specific reason why this wouldn't work, is to add a simple MonoBehaviour script to the dialog prefab, then fire an event whenever it is created. See Science Relay:

https://github.com/DMagic1/KSP_Science_Relay/blob/master/Source/ScienceRelay.cs#L167-L174 and https://github.com/DMagic1/KSP_Science_Relay/blob/master/Source/ScienceRelayDialog.cs

When you process the results dialog you are searching for ModuleScienceExperiment, it might be better to use IScienceDataContainer.

You can use it to determine if an experiment is rerunnable and to get the vessel, and you can get the transmit value from the results dialog.

It looks like the only sticking point is setting the experiment inoperable, which you could probably handle by just checking if the IScienceDataContainer is a ModuleScienceExperiment and going from there.

And, from the forum post on the issue, continuously firing the science transmission event is probably a bad idea. I'm not sure about other mods, but Orbital Science uses it for contracts and for a couple of other things (none of which are particularly demanding), and Surface Experiment Package uses it in a way that could cause problems if it is constantly being fired.

ShotgunNinja commented 7 years ago

It looks like you are just constantly checking to see if the dialog is active from Update. A better way to handle it, unless there is some specific reason why this wouldn't work, is to add a simple MonoBehaviour script to the dialog prefab, then fire an event whenever it is created. See Science Relay:

Thanks I'll look into ScienceRelay. It would definitely be more 'unity' to do that, and in general I should leverage its architecture better. But a part of me just love immediate-mode style...

When you process the results dialog you are searching for ModuleScienceExperiment, it might be better to use IScienceDataContainer.

You can use it to determine if an experiment is rerunnable and to get the vessel, and you can get the transmit value from the results dialog.

It looks like the only sticking point is setting the experiment inoperable, which you could probably handle by just checking if the IScienceDataContainer is a ModuleScienceExperiment and going from there.

I can't agree more on this. I should have done it like this from the start, and you guessed right I went for ModuleScienceExperiment so that I could set it inoperable.

And, from the forum post on the issue, continuously firing the science transmission event is probably a bad idea. I'm not sure about other mods, but Orbital Science uses it for contracts and for a couple of other things (none of which are particularly demanding), and Surface Experiment Package uses it in a way that could cause problems if it is constantly being fired.

My plan for this is to only fire the OnScienceRecovered event when the whole file is transmitted. I will 'buffer' the data amount somewhere, to pass the correct one to OnScienceRecovered. That should deal with the issues that surfaced (and the similar ones that are probable to surface...), while keeping the science being credited constantly (that I think is an interesting aspect of the whole 'background data transmit' business).

The alternative is to only credit the whole science when the full file is transmitted. That is plan B, in case the above doesn't work.

ShotgunNinja commented 7 years ago

I've implemented what discussed above, in the last experimental version.

Data is obtained from IScienceDataContainer. I deal with the resettable experiments as suggested. This lead to supporting virtually any science experiment part added by any mod. I assumed non-ModuleScienceExperiment containers data to be transmissible (funny how that info is stored in the experiment part, instead of the experiment definition) and the experiment repeatable. Both should be safe assumptions I think.

The science crediting instead is 'buffered', and the OnScienceReceivedevent fired once every few Mb of data transmitted. This should minimize issues with OnScienceReceived listeners that are not expecting to be called at 60hz.

The dialog hijacking was kept in 'immediate-mode' style due to laziness.

Closing this. Thanks a lot for the help!