[KSP 1.5] Problems with Stage Icons in mod (Support from KSP Devs is needed)

[jrodriguez]

Hi!

I belong to the BDArmory team. I was doing some tests on KSP 1.5 yesterday and I have found some issues. We have some code for weapons that creates stage icons for weapons. However an Exception is raised in KSP 1.5. I would like to have some feedback from KSP dev team regarding this. I will attach the code that I think may be causing this.

Exception log:

EXC 20:43:43.716] ArgumentOutOfRangeException: Argument is out of range.
Parameter name: index
    System.Collections.Generic.List`1[KSP.UI.Screens.StageGroup].get_Item (Int32 index)
    KSP.UI.Screens.StageManager.AddHeldIconsToStages (.Part selectedPart, Boolean switchingVessels)
    KSP.UI.Screens.StageManager.SortIconsSequence (.Part p, Boolean switchingVessels)
    KSP.UI.Screens.StageManager+<SortIconsSequence>c__Iterator0.MoveNext ()
    UnityEngine.SetupCoroutine.InvokeMoveNext (IEnumerator enumerator, IntPtr returnValueAddress)

Edited October 16, 2018 by jrodriguez

[Ruedii]

A few quick sanity checks first before you pester the KSP devs:
This is an issue with DLLs?
This appears to be an issue with porting to Win64 architecture (i.e. changes to the INT and Pointer typings).

Are you ALWAYS handing ints and int32?
Are you moving an Int32 to an Int and back to an Int32?  
Are you moving a pointer into an Integer of any sort?

While Int32 should be type-compatible with Int and Long, there are options in the compile template in Win64 to override this standard.  This results in various issues.
They are not type compatible with Pointers. 

Simply put, never cross types.   If it's a pointer, ALWAYS use a pointer, if it's an Int, ALWAYS use an Int, if it's a Long, ALWAYS use a Long, and if it's an INT32 always use an INT32.   NEVER assume they are compatible even if they traditionally are.

I hope that helps.   If not, default to the devs.

 

[jrodriguez]

@Ruedii I don't think this issue has nothing to do with x86 x64 architecture.

My feeling with the  ArgumentOutOfRangeException is that the code is iterating through a stagegroup icon list however is accounting for more icons than the icons that are actually in the list.

My question for the devs is:

¿Have something changed between 1.4.5 and 1.5 releases in regards of StageManager class and the way the stage icons are managed?

Here you can see the code we are using to create the icon, this has been working fine until KSP 1.5.

https://github.com/PapaJoesSoup/BDArmory/blob/master/BDArmory/Modules/ModuleWeapon.cs#L773

if (part.stackIcon.StageIcon == null)
{
	part.stackIcon.CreateIcon();
}

 

 

[Ruedii]

3 hours ago, jrodriguez said:

@Ruedii I don't think this issue has nothing to do with x86 x64 architecture.

My feeling with the  ArgumentOutOfRangeException is that the code is iterating through a stagegroup icon list however is accounting for more icons than the icons that are actually in the list.

My question for the devs is:

¿Have something changed between 1.4.5 and 1.5 releases in regards of StageManager class and the way the stage icons are managed?

Here you can see the code we are using to create the icon, this has been working fine until KSP 1.5.

https://github.com/PapaJoesSoup/BDArmory/blob/master/BDArmory/Modules/ModuleWeapon.cs#L773

if (part.stackIcon.StageIcon == null)
{
	part.stackIcon.CreateIcon();
}

 

 

Well, staging icons weren't really designed to be added and removed dynamically, and thus should not be considered an operation to do safely.  Running part.stackIcon.CreateIcon while the game is inflight is an unsafe operation.  The game pauses to avoid unsafe behavior when handling part detach and part destruction removal of staging icons, and resetting the staging order when docking.  Pausing the game frequently is not something I would recommend.

The way I would recommend fixing the issue is just doing it in a manner more inline with standard KSP behavior.

You should instead take parachutes as a model for behavior. 
You should have the icon attached to the base tree of the part from KSP's startup.  (This can be inserted by your module).  For any state indicator you should change to color of the icon.  You should never have to remove or add an icon add an icon.

The icon should be added in the parent object that attaches in automatically to every BDArmory object with the specified capability. 

It should be attached automatically before vessel-load and only be removed when the part is removed from the craft, and KSP automatically removes icons for destroyed and detached parts, so you shouldn't have to worry about this.

In other words, this code shouldn't be necessary at all, because your mod should be written so that the icon exist attached to the part at least from the VAB, if not from game startup.

[Ruedii]

I'm going to look deeper into your exact issue, though.  I found the documentation, and I'm going to RTFM when I get home this afternoon.  I'm already running late.

However, I highly recommend moving to a behavior that doesn't create and destroy staging icons inflight, if only for the sake of optimization.  The create routine for stage icons wasn't intended to be run on a frequent basis, and thus will likely lag you a lot.

It seems that a null comparison is fine in C# to detect the instance of an object.  I personally don't like that method of doing things, but I didn't create the programming language.

Edited October 16, 2018 by Ruedii
null comparison note.

[jrodriguez]

Thanks for the hints @Ruedii I will analyse the behaviour of Parachutes.

Given your knowledge of internals I was wondering if you are KSP developer?

[Ruedii]

No, and in fact I just read the API documentation a few days ago.

I'm just really familiar with what causes problems in object oriented code.  (Sort of a natural with object oriented and prototype-oriented code). 

BTW, there is a switch to hide and unhide stage icons, while it also isn't used in-flight anywhere, it is far more likely to be safe, as it retains the staging order objects were placed in.   You probably should see about using that.  This way you aren't destroying and creating objects dynamically in-flight, which always slows stuff down.  Simply hiding and unhiding always works better than creating and destroying. 

I also noted you were not assigning staging order to objects before creating the icon.  That might be the issue.   Recent compiler changes in Unity made null and void no longer equal zero in a lot of objects, and I noticed that KSP had a unity version release.  Still, the easiest way to deal with this is just assign a deactivated staging icon to every single part in the game during game loading (all the way back at the loading screen before the title screen.)

Edited October 17, 2018 by Ruedii
forgot void