VisualPinball / VisualPinball.Engine.Mpf

Mission Pinball Framework support for VPE
MIT License
0 stars 1 forks source link

Argument Null Exception on startup #19

Open arthurkehrwald opened 1 month ago

arthurkehrwald commented 1 month ago

When starting the table in the editor with an MpfGameLogicEngine component containing a valid machine description attached, the OnInit function in MpfGameLogicEngine.cs throws the following exception:

ArgumentNullException: Value cannot be null.
Parameter name: key
System.Collections.Generic.Dictionary`2[TKey,TValue].TryInsert (TKey key, TValue value, System.Collections.Generic.InsertionBehavior behavior) (at <51fded79cd284d4d911c5949aff4cb21>:0)
System.Collections.Generic.Dictionary`2[TKey,TValue].set_Item (TKey key, TValue value) (at <51fded79cd284d4d911c5949aff4cb21>:0)
VisualPinball.Engine.Mpf.Unity.MpfGamelogicEngine.OnInit (VisualPinball.Unity.Player player, VisualPinball.Unity.TableApi tableApi, VisualPinball.Unity.BallManager ballManager) (at C:/repos/futurebox/reference_and_learning/VisualPinball.Engine.Mpf/VisualPinball.Engine.Mpf.Unity/Runtime/MpfGamelogicEngine.cs:73)
VisualPinball.Unity.Player.Start () (at ./Library/PackageCache/org.visualpinball.engine.unity@0.0.1-preview.130/VisualPinball.Unity/VisualPinball.Unity/Game/Player.cs:177)

The Id property of all switches, coils and lights, is set to null when the game is started. This is also visible in the Inspector. Before clicking play: Screenshot 2024-07-14 173009 After clicking play: Screenshot 2024-07-14 173104

I am using:

arthurkehrwald commented 1 month ago

I tried to track down what causes the ids to be set to null by debugging, but I can't figure it out. Nothing seems to change the ids or replace the switches in the array on the MPF game logic engine, they're just null as soon as the game starts

arthurkehrwald commented 1 month ago

I found the problem. This commit in the main VPE repository changed the access specifier of the switch, coil and lamp IDs from public to private. This causes Unity to no longer serialize their values. From the Unity documentation:

To use field serialization you must ensure that the field:

I can think of the following options to fix this: Option Downside
Make the properties public again Violates encapsulation principle, A (bad) design decision made because of an implementation detail of Unity in a project that should not be dependent on it
Add the [SerializeField] attribute to the properties Introduces a dependency on Unity to the core part of VPE that is not supposed to have third-party dependencies
Make the properties protected, create new subclasses of GameLogicEngineCoil, GameLogicEngineSwitch and GameLogicEngineLamp in the VisualPinball.Unity project and implement Unity's ISerializationCallbackReceiver interface to serialize the protected properties of their respective parent classes Maintenance burden, increased complexity

I think the downsides of the last option are the least severe.

On top of this, when the machine description is updated via the inspector, the Unity Editor must be notified of the changes for them to be saved with the scene.

I have created pull request #20 in this repository and this pull request in the main VPE repository to implement the proposed changes and solve this issue.