Conversation
There was a problem hiding this comment.
Pull request overview
Updates the VPE PinMAME integration to a newer PinMAME/.NET wrapper + native runtime, and adjusts Unity plugin deployment to match the new binary names/layout.
Changes:
- Bump PinMAME managed + native package versions and centralize them as MSBuild properties.
- Update plugin deployment to copy
PinMameDotNet.dlland handle Windows x64 native rename (pinmame64.dll→pinmame.dll). - Add/update Unity plugin
.metafiles for the new native/runtime artifacts and refresh README/.gitignore guidance.
Reviewed changes
Copilot reviewed 5 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| VisualPinball.Engine.PinMAME/VisualPinball.Engine.PinMAME.csproj | Updates PinMAME package versions and modifies the MSBuild plugin deployment (including win-x64 native rename). |
| VisualPinball.Engine.PinMAME.Unity/Runtime/PinMameGamelogicEngine.cs | Makes switch handling more defensive and lowers switch logging verbosity. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/win-x86/PinMameDotNet.dll.meta | GUID updated for the Win x86 managed wrapper plugin meta. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/win-x86/PinMame.dll.meta | GUID updated for the Win x86 plugin meta. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/win-x64/pinmame.dll.meta | Adds meta for renamed Win x64 native library Unity should load as pinmame.dll. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/win-x64/PinMameDotNet.dll.meta | Adds meta for Win x64 managed wrapper plugin. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/osx/libpinmame.3.7.0.dylib.meta | Adds meta for macOS native library. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/osx/PinMameDotNet.dll.meta | Adds meta for macOS managed wrapper plugin. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/linux-x64/libpinmame.so.3.7.0.meta | Adds meta for Linux native library. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/linux-x64/PinMameDotNet.dll.meta | Adds meta for Linux managed wrapper plugin. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/ios-arm64/libpinmame.3.7.0.a.meta | Adds meta for iOS static library. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/ios-arm64/PinMameDotNet.dll.meta | Adds meta for iOS managed wrapper plugin. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/android-arm64-v8a/libpinmame.3.7.0.so.meta | Adds meta for Android native library. |
| VisualPinball.Engine.PinMAME.Unity/Plugins/android-arm64-v8a/PinMameDotNet.dll.meta | Adds meta for Android managed wrapper plugin. |
| README.md | Updates local-dev guidance for which plugin .meta files to mark assume-unchanged after version bump. |
| .gitignore | Changes ignore behavior for Plugins/ content. |
Comments suppressed due to low confidence (1)
VisualPinball.Engine.PinMAME.Unity/Plugins/win-x86/PinMameDotNet.dll.meta:2
- Changing an existing Unity asset GUID can break references in downstream Unity projects. This PR swaps the GUIDs between
PinMameDotNet.dll.metaandPinMame.dll.meta(the files are otherwise identical), which risks invalidating any references to these plugin assets. If the intent was to reflect a DLL rename, prefer moving/renaming the.metafile together with the renamed DLL so the GUID stays stable for the same logical asset.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (_switches.TryGetValue(id, out var sw)) { | ||
| if (_switchIdToPinMameIdMappings.TryGetValue(sw.Id, out var pinMameId) && _mechSwitches.Contains(pinMameId)) { | ||
| // mech switches are triggered internally by pinmame. | ||
| return; | ||
| } | ||
| if (_pinMame != null && _isRunning) { | ||
| if (Logger.IsDebugEnabled) { | ||
| Logger.Debug($"[PinMAME] => sw {id}: {isClosed} | {sw.Description}"); | ||
| } | ||
| if (_switchIdToPinMameIdMappings.TryGetValue(sw.Id, out pinMameId)) { | ||
| _pinMame.SetSwitch(pinMameId, isClosed); | ||
| } |
There was a problem hiding this comment.
In Switch, if a switch exists in _switches but has no entry in _switchIdToPinMameIdMappings, the method silently does nothing (no log, no call to _pinMame.SetSwitch), yet still fires OnSwitchChanged. This can make mapping/config issues hard to diagnose and can desync consumers from the PinMAME state. Consider logging a warning/error when the mapping is missing and aligning SendInitialSwitches() (which still indexes into _switchIdToPinMameIdMappings) to avoid potential KeyNotFoundExceptions for the same condition.
| @@ -1,5 +1,5 @@ | |||
| fileFormatVersion: 2 | |||
| guid: 9a12abde327b949e699c61497c4b2618 | |||
| guid: 65f3dcac0af3b4ff5b00f2edf91fe406 | |||
There was a problem hiding this comment.
Changing an existing Unity asset GUID can break references in downstream Unity projects. This PR swaps the GUIDs between PinMame.dll.meta and PinMameDotNet.dll.meta (the files are otherwise identical), which risks invalidating any references to these plugin assets. If the intent was to reflect a DLL rename, prefer moving/renaming the .meta file together with the renamed DLL so the GUID stays stable for the same logical asset.
1 participant
No description provided.