lushen124 / Universal-FE-Randomizer

Properly universal this time.
MIT License
98 stars 28 forks source link

Refactoring of the GBARandomizer #407

Open Geeene opened 1 year ago

Geeene commented 1 year ago

This pull request has a Refactor of the GBARandomizer class to make it easier to manage.

Rather than having a single class for all 3 GBA Games, including the exception cases for each one, it is now split into four separate classes:

AbstractGBARandomizer - for Shared implementations and the generic process flow

FE6Randomizer - for FE6 Specific Implementations such as applying the translation patch FE7Randomizer - for FE7 Specific Implementations such as Mode Selection Changes FE8Randomizer - for FE8 Specific Implementations such as Trainee Seals

While this is technically not shorter when it comes to the number of lines, that is in part due to additional JavaDoc and Formatter Changes.

One big advantage however is that the generic flow used by all three randomizers is now quite simple to understand: 2023-02-20 19_17_27-Yune - Universal FE Randomizer_src_random_gba_randomizer_AbstractGBARandomizer j

And people who want to do changes for FE6 / FE8 don't have to scroll through 200+ Lines of ensuring that Hector can win against Wire in Chapter 11, as that will be hidden away in the FE7Randomizer ;)

Some other small improvements:

Adding a new Randomization feature is now as simple as doing this: 2023-02-20 19_25_48-Yune - Universal FE Randomizer_src_random_gba_randomizer_AbstractGBARandomizer j

rather than this: 2023-02-20 19_25_14-Yune - Universal FE Randomizer_src_random_gba_randomizer_GBARandomizer java - Ec

As all the exception handling logic is now centrally handled. 2023-02-20 19_28_22-Yune - Universal FE Randomizer_src_random_gba_randomizer_AbstractGBARandomizer j

Additionally I have moved the responsibility to record the options into the Option classes rather than as part of the Randomizer. I have added an interface, RecordableOption.

So an Option Model should now implement this Interface and implement it's method so that the Randomizer can rid itself of the responsibility to do it for them.

2023-02-20 19_31_01-Yune - Universal FE Randomizer_src_random_gba_randomizer_AbstractGBARandomizer j

Interms of functionality, there really shouldn't be a big difference, it's mainly restructured to be easier to maintain.

lushen124 commented 1 year ago

This is great! The initial design was intended to try to reuse as much code as I could since GBAFE was fairly similar when I started it, but as there was more and more special cases to handle between each GBA game, it started to become a bit unwieldy, but I never got around to refactoring it.

Since we're doing this, we should try to get it merged as soon as we can so future work on GBAFE will be built on a much better foundation.

Thank you for taking on such a large effort! 🙂

Geeene commented 1 year ago

Good to know that you like it, I have used it quite a bit when I was working on merging my own master branch with all the features I have implemented so far, and I guess one thing that is still somewhat annoying with this refactor, is that whenever you need to add an option class to the Randomizer you have to add it as a parameter to like 4 constructors, from thinking about it a little, it surely would be possible to reuse the bundles that are used to save the settings between restarts, as they also already contain the same information as what is passed for the constructor, so I'll have a look at that

Geeene commented 1 year ago

Are you opposed to saving the Option Bundle before the Randomization happens?

i.e. like this?

2023-04-07 10_30_39-Yune - Universal FE Randomizer_src_ui_MainView java - Eclipse IDE

Or would it be better to create the instance of the GBAOptionBundle without saving first, pass that into the randomizer, and then after the randomization it gets saved?

lushen124 commented 1 year ago

Go for it. The order doesn't really matter, and I'd argue it's probably better to save as soon as possible to make sure we don't lose user options if something goes wrong.

Geeene commented 1 year ago

Merged your master into the branch and did the neccessary adjustments to match the new structure. Also passed the GBAOptionBundle to the Constructor now, so that any new options don't have to be added to 5 different constructors but rather just 2, the factory, and the shared constructor:

AbstractGBARandomizer.buildRandomizer(String, String, GameType, DiffCompiler, GBAOptionBundle, String) AbstractGBARandomizer.AbstractGBARandomizer(String, String, GameType, DiffCompiler, GBAOptionBundle, String, String)

Geeene commented 1 year ago

I guess technically there would be the least overhead if we merge this one last, as then it's just one merge, while with merging this first we have to merge every other pull request that changes the GBARandomizer