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

Adds FX3 colorization support #367

Merged
merged 21 commits into from
Apr 21, 2023
Merged

Conversation

GyroJoe
Copy link
Contributor

@GyroJoe GyroJoe commented Apr 16, 2023

Adds support for colorizing FX3 frames when using the memory grabber.

To achieve this, a new memory grabber has been added that can determine the name of the current game. (It appears to be the name for the UI skin the menu uses). These names are the same as the ones used for backglass images (e.g. WMS_Monster_Bash).

Once the game name is determined, the palette is loaded, and switched to dynamically. In this way, new colorizations can be loaded without tearing down the entire render graph.

The same altcolor folder location strategy is used as for VPinMame. It also now searches the COM location, using the same trick the installer does.

For now, I only added support for PAL/VNI, but Serum should be feasible - need a colorization for a supported game to test with. I know there has been some churn in the colorization space lately - I tried to avoid changing the logic as much as possible and only moved things to a new ColorizationLoader so the code could be shared. Hopefully this is not disruptive to any on-going refactor work.

@GyroJoe
Copy link
Contributor Author

GyroJoe commented Apr 16, 2023

Quick screenshot of it in action:
image

@zesinger
Copy link
Contributor

That's really amazing! Nice addition, thanks! I'd be happy to help for Serum,

@freezy
Copy link
Owner

freezy commented Apr 16, 2023

Dude, you're killing it! Thanks!

I haven't started the coloring refactoring yet, so I'll review and merge this one first. Cheers!

@freezy
Copy link
Owner

freezy commented Apr 16, 2023

Had a quick test. Nice!

image

I'll finish the review later today and push some minor changes. Would be awesome to have this integrated with the other sources as well (FP, PBA).

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.

This is great. A few more things before we merge this:

  • Fix the color hue when coloring is disabled, and enabled but no colorization available
  • Add the same feature to Future Pinball and Pinball Arcade
  • Document the feature in README.md
  • Test Serum and PinMAME

I'll have a go at the second and last point.

LibDmd/Converter/SwitchingConverter.cs Outdated Show resolved Hide resolved
LibDmd/Converter/SwitchingConverter.cs Show resolved Hide resolved
LibDmd/Input/PinballFX/PinballFX3MemoryGrabber.cs Outdated Show resolved Hide resolved
@freezy
Copy link
Owner

freezy commented Apr 16, 2023

Serum now working.

image

@zesinger
Copy link
Contributor

Amazing! Thanks

README.md Outdated
@@ -140,6 +140,20 @@ The DMD from Pinball FX3 is pulled directly from the memory.
It doesn't matter whether Pinball FX3 is started before or after `dmdext`, and
it works with or without cabinet mode.

Like VPM, the FX3 memory grabber also supports colorization on a monitor or RGB display.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if we should move all the info on colorization to their own section, rather than having it under each game, since there is a fair amount of overlap.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, that's a good idea. Also, add a table of contents :)

@freezy
Copy link
Owner

freezy commented Apr 16, 2023

Pinball Arcade is somewhat working, but I had to change the frame format, so 4-bit games don't work right now. Will see if I find a fix and push tomorrow.

image

@freezy
Copy link
Owner

freezy commented Apr 16, 2023

Oh, and I just created an IGameNameSource interface and made PBA implement it. @GyroJoe is there any reason you've created a completely new grabber for the name, instead of adding this to the existing one?

@GyroJoe
Copy link
Contributor Author

GyroJoe commented Apr 17, 2023

I mainly didn't love the idea of that one grabber doing so many different things. It's also running at a different framerate (1 FPS, lol), since the game name is infrequently changing.

@GyroJoe
Copy link
Contributor Author

GyroJoe commented Apr 17, 2023

Should fix #344, and at least the color part of #258

@freezy
Copy link
Owner

freezy commented Apr 17, 2023

I mainly didn't love the idea of that one grabber doing so many different things. It's also running at a different framerate (1 FPS, lol), since the game name is infrequently changing.

Okay. I might move it into the original grabber, because a) it allows to abstract the name retrieval across sources and b) have less duplicated code. I'll measure the performance penalty, but my guess is that's it's negligible.

@freezy
Copy link
Owner

freezy commented Apr 17, 2023

I've pushed the TPA update. @GyroJoe let me know what you think. I didn't touch FX3 yet, but basically you first set up the graphs, and at the end the switching converter, if the source supports it.

@freezy freezy mentioned this pull request Apr 18, 2023
@GyroJoe
Copy link
Contributor Author

GyroJoe commented Apr 18, 2023

I've pushed the TPA update. @GyroJoe let me know what you think. I didn't touch FX3 yet, but basically you first set up the graphs, and at the end the switching converter, if the source supports it.

Clean, and unifies the setup across sources. I implemented this for FX3 and added a new type IDmdColorSource.

One wrinkle with the unified grabber for FX3 is that the game name and the DMD aren't both available at the same point in time. I added a one-time second attempt to get the name if it fails the first time through. This was one advantage of the previous split system I guess, so I never noticed.

@freezy
Copy link
Owner

freezy commented Apr 18, 2023

Awesome, thanks a lot! Looks much cleaner indeed.

Also thanks for updating .editorconfig, I might tweak it a little more.

@freezy
Copy link
Owner

freezy commented Apr 18, 2023

So the remaining tasks are Future Pinball, although it's currently only emitting 4-bit frames, which I'd have to convert, and I don't know how accurate the animations and texts are so they'd be recognized by the coloring plugins (FP tables aren't ROM based, so authors re-create the DMD output manually). I'll also spend some time on reorganizing the documentation a little.

@GyroJoe you don't happen to be interested in working on a new pinball simulator based on Unity? ;)

@GyroJoe
Copy link
Contributor Author

GyroJoe commented Apr 18, 2023

Also thanks for updating .editorconfig, I might tweak it a little more.

A word of warning, the editor in VS for this file is pretty broken. Just change it with a normal text editor.
dotnet/roslyn#54556

@freezy
Copy link
Owner

freezy commented Apr 20, 2023

For me it's ready to merge now. @GyroJoe if you wanna have a last look, lemme know, otherwise I'll go ahead.

@GyroJoe
Copy link
Contributor Author

GyroJoe commented Apr 21, 2023

Wow, you went the extra mile with the docs including all the game names.

🚢 it

@freezy freezy merged commit a5dfd6f into freezy:master Apr 21, 2023
@GyroJoe GyroJoe deleted the fx3-colorization branch April 21, 2023 07:22
@lucky01
Copy link
Contributor

lucky01 commented Apr 21, 2023

@GyroJoe Great new feature ! Thanks for that !
@frezzy I would appreciate instead of making a release with this new feature based on deprecated vni/pal support, to make the next release together with the refactored plugin interface and enable pac support that way. Otherwise it encourages the community to continue to illegally share vni/pal colorization against the will of the authors. We should not repeat history here.

@freezy
Copy link
Owner

freezy commented Apr 21, 2023

@lucky01 That's an absurd suggestion. This feature was already announced on several Discords, how long do you think it'll take to spread on the forums anyway? It's an open source project, remember.

@lucky01
Copy link
Contributor

lucky01 commented Apr 21, 2023

@freezy O.K. no problem ! I thought I just ask before I pull the changes into my fork.

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.

4 participants