freezy / VisualPinball.Engine

:video_game: Visual Pinball Engine for Unity
https://docs.visualpinball.org
GNU General Public License v3.0
396 stars 62 forks source link

Implement Score Motor component #435

Closed jsm174 closed 1 year ago

jsm174 commented 2 years ago

This PR closes #421.

It adds a new ScoreMotorComponent that can be configured to correctly one or many ScoreReelDisplayComponent.

ScoreReelDisplayComponent has been updated to look for a ScoreMotorComponent. If found, the display will keep track of its score.

The Score Motor can be configured in the inspector:

Screen_Shot_2022-08-26_at_5 29 26_PM

The default settings are suitable for a Gottlieb EM, ie: 6 steps in 120 degrees over 769ms.

The component forces a working configuration, ie, you can not configure less than 5 steps.

The ScoreMotorComponent also exposes two switches Motor Running Switch and Motor Step Switch that can be configured in the Switch Manager:

Screen_Shot_2022-08-26_at_5 28 51_PM

There will be a follow up PR that adds new Visual Scripting Units to tie this all together.

Thanks again to @freezy, @Scottacus64, @bord, and @Cupiii for the several hours of discussion to get this figured out.

Scottacus64 commented 2 years ago

This looks awesome! Thanks for all of the hours you put into this!

jsm174 commented 2 years ago

After discussions on Discord, @freezy suggested using the existing Update Display unit instead of an additional Add Points unit.

I've refactored the code accordingly, and it really makes everything a lot more clean. The ScoreReelDisplayComponent now keeps track of its score internally.

I've also added selecting the ScoreMotorComponent directly on the ScoreReelDisplayComponent. This would allow us to have multiple score motors with different configurations for testing:

Screen Shot 2022-08-28 at 2 11 08 PM

If a ScoreMotorComponent is not attached to the ScoreReelDisplayComponent, everything works exactly like it did before.

jsm174 commented 2 years ago

The last remaining issue is to revisit the score reel animation. It can often times get out of sync with the reels internal score.

jsm174 commented 2 years ago

Added a few comments. One thing I still don't understand is why can't you completely replace the add points methods by set point methods?

The reason why I'm using add points is because of the blocking.

For example, we have 1000 points and we tell the score display to update 500 points:

Then you can also unify it with reset. I still don't get why reset is any different from "set points to 0".

Since we had the Update Display visual scripting unit, and each display offers a clear method, I thought a Clear Display unit would be a good addition.

Reset is doing a little more than just setting the reels to zero. It follows all the same timing rules, and has logic to keep the motor running for an additional turn if needed.

Say the score is 2230, one turn is going to advance the digits 5 times:

3340, 4450, 5560, 6670, 7780

We still need to respect that timing (wait, increase, increase, increase, increase, increase).

Since we are not at zero, the score motor continues for another revolution

8890, 9900, 0, 0, 0

(wait, increase, increase, increase, increase, increase).

freezy commented 2 years ago

Good point, I didn't think about the concurrency issue. Now it makes sense!

I'll go through it again tonight and merge if all is good.

Thanks for bearing with me :)

freezy commented 2 years ago

Oh, one more thing.

I suppose that currently, in UVS, it's entirely possible for a user to put scores like 1500. How do we deal with that? Error, warning, or fall back to non-score-motor driven scoring for those cases?

jsm174 commented 2 years ago

Good point, I didn't think about the concurrency issue. Now it makes sense!

I'll go through it again tonight and merge if all is good.

Thanks for bearing with me :)

I went through and cleaned up the DisplayUpdateEvent to just DisplayEvent. Makes it more like SwitchEvent CoilEvent etc.

I also removed DisplayPlayer from Displays. They now use an OnDisplayChanged event.

jsm174 commented 2 years ago

Oh, one more thing.

I suppose that currently, in UVS, it's entirely possible for a user to put scores like 1500. How do we deal with that? Error, warning, or fall back to non-score-motor driven scoring for those cases?

The score motor figures out the increase doesn't make sense and then logs the error, and the points are thrown away.

if (increase > ScoreMotorComponent.MaxIncrease) {
   Logger.Error($"too many increases (ignoring points), id={id}, points={points}, increase={increase}");
   return;
}
freezy commented 2 years ago

Oh, and don't forget the UVS doc for the new nodes!

freezy commented 2 years ago

Also, PinMAME needs to be updated with the new API (and preferably MPF as well)