Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CollisionSwitchComponent #344

Merged
merged 12 commits into from
Nov 24, 2021
Merged

Add CollisionSwitchComponent #344

merged 12 commits into from
Nov 24, 2021

Conversation

jsm174
Copy link
Collaborator

@jsm174 jsm174 commented Nov 21, 2021

This component will expose switches for the following components that inherit IApiHittable

This was needed as there are switches behind the drop target banks and non-slingshots in Rock.

This PR also does the following:

  • updates drop target bank component to support 10 drop targets
  • updates all projects to netstandard2.1 and C# 9.0
  • fixes VisualPinball.Engine.Unity.sln compilation errors using Visual Studio.
  • updates FluentAssertions to 6.2.0
  • updates .unity folder with latest managed dlls for compiling in Visual Studio without Unity

(Copied from /Applications/Unity/Hub/Editor/2021.2.3f1/Unity.app/Contents/Managed/UnityEngine and ScriptAssemblies)

Remaining:

  • Create new icons for CollisionSwitch (@freezy)
  • Support more components other than surface, primitive, and rubber? (Now supports IApiHittable)
  • Figure out why manager isn't showing enabled for some of the hits while it is registering in PinMame. See Add CollisionSwitchComponent #344 (comment)

@jsm174 jsm174 requested a review from freezy November 21, 2021 16:21
@codecov
Copy link

codecov bot commented Nov 21, 2021

Codecov Report

Merging #344 (0a05e33) into master (292aca1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #344   +/-   ##
=======================================
  Coverage   83.91%   83.91%           
=======================================
  Files         125      125           
  Lines        6741     6741           
=======================================
  Hits         5657     5657           
  Misses       1084     1084           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 090129e...0a05e33. Read the comment docs.

@jsm174
Copy link
Collaborator Author

jsm174 commented Nov 23, 2021

So I had a chance to look at the switch manager status.

Screen Shot 2021-11-22 at 9 45 55 PM

Since this same switch ID is currently hooked to 6 collision switches, the status is only updated when one of the collision switches is enabled.

It looks like the switch statuses are stored in a dictionary by ID. Additional switches with the same ID, just override the dictionary value. So the last component is the one that is used for the status:

if (deviceSwitch != null) {
var switchStatus = deviceSwitch.AddSwitchDest(new SwitchConfig(switchMapping));
SwitchStatuses[switchMapping.Id] = switchStatus;

Thoughts on this?

@freezy
Copy link
Owner

freezy commented Nov 23, 2021

Thoughts on this?

That's okay, no? I agree it's a bit weird in the editor, but any of those elements triggers the same switch, so it's kinda logical that they all change status synchronously.

The other way of handling this would be to additionally store switch statuses in a switch mapping -> status dictionary, just for the editor. I'd create a GitHub issue about this and move along. :)

Copy link
Owner

@freezy freezy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it's good to merge!

@jsm174
Copy link
Collaborator Author

jsm174 commented Nov 24, 2021

That's okay, no? I agree it's a bit weird in the editor, but any of those elements triggers the same switch, so it's kinda logical that they all change status synchronously.

Sorry, I guess I didn't explain it properly. I'll create another issue.

Switch 41 is currently used in 8 places, for example:

  • A (Behind upper drop target bank)
  • B (Behind lower drop target bank)
  • C (Upper left side rubber)
  • D (Upper right side rubber)
  • E (Middle left side rubber)
  • F (Middle right side rubber)
  • G (Left slingshot - FUTURE)
  • H (Right slingshot - FUTURE)

If the user hits A, all 41 entries in the Switch Manager close. Totally fine.

Screen Shot 2021-11-23 at 6 51 53 PM

However, if the user hits B, C , D, E, F, G, H, the entries in the switch manager will not close.

The Game Logic Engine will work fine and PinMame registers the switch for A - H.

The SwitchPlayer statuses are a dictionary by ID 41

if (deviceSwitch != null) {
var switchStatus = deviceSwitch.AddSwitchDest(new SwitchConfig(switchMapping));
SwitchStatuses[switchMapping.Id] = switchStatus;

So when the player is populating the dictionary, each ID 41 switch device overwrites the last one.

So that's why the manager looks like its only monitoring one device.

@jsm174 jsm174 merged commit a1cef25 into master Nov 24, 2021
@jsm174 jsm174 deleted the rock-updates branch November 24, 2021 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants