search

shadowmage45 / SSTULabs

Dev repository for testing/unfinished KSP parts/plugins/etc.
Other
62 stars 41 forks source link

Kerbalism support : Kopernicus multi-star handling #803

Closed gotmachine closed 5 years ago

gotmachine commented 5 years ago

Moved the tracked CelestialBody to a top-level variable. This allow Kerbalism to change it trough reflection for its in-house Kopernicus multi-star support.

shadowmage45 commented 5 years ago

Seems reasonable. My only question is -- will the bodies[] list be populated at the time of this call? : https://github.com/shadowmage45/SSTULabs/pull/803/files#diff-2da0507f8787c7b7e57dca884cdb392bR105

I'm unsure if the FlightGlobals.bodies[] list is static, or rebuilt on every scene-change. If static, then no worries. If rebuilt, then need to ensure proper order-of-setup, and that flight-globals is fully instantiated and configured prior to grabbing the reference.

gotmachine commented 5 years ago

FlightGlobals.bodies[] is not static but it's populated in the PSystemManager.Start() method, which I believe is called very early as it's responsible for spawning the whole system. I've never seen a case where it's not available.

shadowmage45 commented 5 years ago

Sounds good.

I'll be (hopefully) pushing out an update to SSTU this weekend, so please let me know if you encounter anything else that needs hooks.

gotmachine commented 5 years ago

You were right, this is causing a (not very harmful, but still) issue because the SolarModule ctor is called in OnLoad, not only in OnStart. This cause it to be called on the prefab creation at loading, when the FlightGlobals/PSystemManager aren't yet ready, resulting in nullref spam at loading. Instead of introducing more complexity to handle that, I changed the variable to be the int flightglobal index of the CelestialBody instead of a direct reference to the CB object, hope this is fine for you.

shadowmage45 commented 5 years ago

Hmm.. yeah, could see that being an issue. Not sure of a way to remove the initialization from OnLoad(), as that is needed for prefab configuration during initial part-loading.

Was concerned about unchecked access-by index... but that is already what I was essentially doing by explicitly referencing [0] anyway, so no real loss of safety there.

No real problem with the proposed changes, as long as you think they will be sufficient for your integration needs.

gotmachine commented 5 years ago

Good progress done on integration ;) image

shadowmage45 commented 5 years ago

Excellent; good to see that is working well for you. Will be merging this PR into the dev branch shortly, and will be available with the next SSTU release.

Are there any other Kerbalism integration changes/hooks needed?

It is not pertinent to this PR directly, rather I am asking so as to try and get everything needed in place for integration for the next SSTU release, as I'm not sure how long it will be between updates, and would rather not leave things half-working if it can be avoided.

gotmachine commented 5 years ago

Unfortunately we didn't have time yet to fully create and test our support configs, so maybe there are other issues that could require some code changes, but I hope this PR and the one from @steamp0ort will be enough.

One thing that would be nice to have is for editor part tweaking actions to fire the onEditorPartEvent game event (maybe with the ConstructionEventType.PartTweaked constant) because having to subscribe to onEditorShipModified is a huge pain since this is is fired very often for every editor action, and that there is no way to identify what happened.

Another thing : the "ST-MST-ISS solar truss" is using a separate SSTUSolarPanelDeployable instead of having its solar panel configured in its SSTUModularPart module. It's a bit hard for us to handle this situation as we can't easily detect if a SSTUModularPart has a solar panel or not (It's possible but I want to limit the complexity, this thing is already a mess) Any specific reason for this particular setup ? Would it be possible to have only the SSTUModularPart with a single solar panel option ? I tried to change the configs to do it but I only managed to break the whole part.

shadowmage45 commented 5 years ago

but I hope this PR and the one from @steamp0ort will be enough.

Understood; please let me know if you run into anything else.

fire the onEditorPartEvent game event

Hmm... must be a (recently) new event? Not sure I'm familiar with it, though I haven't poked at the KSP API much since ~1.3 era. Will certainly look into it, as yes, the generic 'ship modified' event sucks for efficiency.

"ST-MST-ISS solar truss" is using a separate SSTUSolarPanelDeployable [...] Any specific reason for this particular setup ?

Not that I can think of/remember off the top of my head. Likely it just got incrementally updated to keep functionality with minimal effort. Hmm... perhaps it was how I needed the cloned panel models to be parented to a specific root transform to facilitate multi-axis tracking, and the ModularPart setup didn't support that.

I'll take a look at it, but I can't promise anything. Is a bit of an odd part and doesn't really fit with the setup used by the rest of the parts, so not sure how to best make it work. Might be possible to do some hackery with the MODEL_DEFINITIONS to keep the functionality in place; define an empty model for the 'core', and a single-position solar layout specific to that part, and then for the solar-panel model definition, include the full pre-welded set of panels+core. We'll see :)

we can't easily detect if a SSTUModularPart has a solar panel or not

How are the DOS and other ModularParts that have optional solar panel configurations handled when the panels are disabled? (just curious)

gotmachine commented 5 years ago

How are the DOS and other ModularParts that have optional solar panel configurations handled when the panels are disabled? (just curious)

Actually I'm now watching the "currentSolar" field changes instead of using the onEditorShipModified event.

As for the handling of things, I'm fully reflection-copying the whole setup (see here). Basically I get references to the suncatchers and pivot transforms, the nominal rate (and set it to 0 so nothing is produced from SSTU modules), and a few other things (like your raytracing occlusion check). I store everything in my own data structure and I use it to calculate the EC output using our common methods that we use for the stock module, the NFS module and yours.

It's a bit inefficient as your module is still running all its own code, but it's needed anyway for the animation handling.

shadowmage45 commented 5 years ago

As for the handling of things, I'm fully reflection-copying the whole setup (see here).

Browsed through it a bit, and have to say that I'm impressed by the amount of effort you are putting into integration.

It's a bit inefficient as your module is still running all its own code, but it's needed anyway for the animation handling.

That is something that might be able to be solved. Usually pretty easy to disable code conditionally from a config-specified value, so could have one line to patch, and enjoy some increased efficiency.

As you note, the entire system can't be disabled outright as it all links together the animations/etc, but certainly all of the calculations and calls to the KSP API could be disabled.

I've opened up another issue ticket on the discussion of that (or other solar-panel integration discussions), so that I can merge and close this PR :) https://github.com/shadowmage45/SSTULabs/issues/807