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

Refactor InputDisplayGenerator + LogEntryGenerator + ControllerDefinition #3782

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Morilli
Copy link
Collaborator

@Morilli Morilli commented Sep 28, 2023

This PR supersedes / expands #3742.
This also partially (fixes) #3779 (the exception is fixed and the movie imports fine, but the logkey is missing FDS controls and so the movie may misbehave when editing it).

The main goals here are to improve performance (e.g. for the movie importers) by improving caching, simplify code and make the structure better and clearer.

Now, the Bk2LogEntryGenerator and Bk2InputDisplayGenerator classes are entirely static and just take an IController as input. They now rely on the IController::Definitions MnemonicCache to be performant, whereas such a cache was previously built by the classes themselves.
I'm not entirely sure about this cache being on ControllerDefinition, but I feel like it does make sense in the current code structure.
The fact that this cache needs to be manually built is kinda ugly. The best solution I can see for that would be to move the Bk2MnemonicLookup to a place accessable by ControllerDefinition and build the mnemonic cache on MakeImmutable (which then needs an additional systemId arg).

Oh, I also removed the hacky SetMovieController function that was used for initial default.tasproj saving, didn't seem to be necessary? this will 100% cause bugs later on

Check if completed:

I can already see this avalanching into 100 different bugs
+ make Bk2LogEntryGenerator and Bk2InputDisplayGenerator static
This should simplify stuff and make the logic clearer
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.

None yet

1 participant