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 Info Display #35

Merged
merged 5 commits into from
Oct 10, 2023
Merged

Add Info Display #35

merged 5 commits into from
Oct 10, 2023

Conversation

ximk
Copy link
Member

@ximk ximk commented Sep 27, 2023

No description provided.

Appease Linter (hopefully)
@@ -0,0 +1,35 @@
[DEBUG]
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if the config file that is used is not tracked by git.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify, this file should ABSOLUTELY be committed. Because we always want to be able to provide it alongside the scripts (We don't expect the user to have to create this file manually themselves). However, we will want to ignore any changes to the file. Is this doable?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would work if this file was the default configuration and is used to supply configuration the main config file lacks.

@kierio04
Copy link
Contributor

kierio04 commented Sep 27, 2023

Would it be more beneficial to have an additional .py file to allow for further customisation using presets? For example:

  • Default (Very basic info; only anything that a beginner might want - Frame Count, IV XYZ, Charges and Boosts, Checkpoints and Completion, Miscellaneous)
  • TASing (More info than Default; what a more experienced TASer might want - Default + Speed, EV XYZ, MRV XYZ, MWV XYZ, Position)
  • Micro (Info specific only to micro-optimisation - Frame Count, Lap Splits, Speed, Checkpoints and Completion, Position)
  • Glitch (Info specific only to glitch hunting - Frame Count, Speed, IV XYZ, EV XYZ, Charges and Boosts, Checkpoints and Completion, Miscellaneous)
  • All (All info available is shown, excepting any additional checks made in the infodisplay script itself, e.g. SMT only displays for karts)
  • Custom (Whatever is defined in the ini file, which is shipped with the same settings as the Default preset)

I'm aware a lot of these have very few differences for now, but as more stuff gets added to infodisplay, more stuff will only cater to certain people/environments, making it useful to have presets to switch between.

@ximk
Copy link
Member Author

ximk commented Sep 27, 2023

Would it be more beneficial to have an additional .py file to allow for further customisation using presets? For example:

  • Default (Very basic info; only anything that a beginner might want - Frame Count, IV XYZ, Charges and Boosts, Checkpoints and Completion, Miscellaneous)
  • TASing (More info than Default; what a more experienced TASer might want - Default + Speed, EV XYZ, MRV XYZ, MWV XYZ, Position)
  • Micro (Info specific only to micro-optimisation - Frame Count, Lap Splits, Speed, Checkpoints and Completion, Position)
  • Glitch (Info specific only to glitch hunting - Frame Count, Speed, IV XYZ, EV XYZ, Charges and Boosts, Checkpoints and Completion, Miscellaneous)
  • Custom (Whatever is defined in the ini file, which is shipped with the same settings as the Default preset)

I'm aware a lot of these have very few differences for now, but as more stuff gets added to infodisplay, more stuff will only cater to certain people/environments, making it useful to have presets to switch between.

Personally, I think an external guide of recommendations for certain scenarios (and just common sense) would negate the need for presets.

@vabold
Copy link
Member

vabold commented Sep 27, 2023

I like the idea of presets. May be out of the scope of this PR.

@malleoz
Copy link
Collaborator

malleoz commented Sep 28, 2023

I agree that presets are nice, but also agree that it is out of scope. Let's try to get this merged with this basic .ini config file.

I believe we are in agreement that the plan is to populate the config object with a ['DEFAULT'] configuration. Upon setting this object, we should then check to see if the config.ini file has been created. If not, we should copy the DEFAULT configurations to the file. Afterwards, the user will be able to see the default info display in-game, and now be able to edit the generated config.ini file to make the appropriate changes.

@kierio04
Copy link
Contributor

kierio04 commented Sep 28, 2023

In that case, I still think we should remove some of the things from being displayed by default. I believe there should only be; Frame Count, Speed, EV XZ/XYZ, Charges and Boosts, Checkpoints and Completion, and Airtime. This means:

  • Adding an Airtime parameter, defaulting it to True
  • Removing Airtime from the Miscellaneous block in create_infodisplay()
  • Defaulting Surface Properties to False
  • Defaulting Position to False

Also, we should decide on some consistency regarding the speed/velocity sections. I believe the following should be our layout:

  • IV X/Y/Z and IV XZ/XYZ as the two parameters to toggle (by default, both False)
  • EV X/Y/Z and EV XZ/XYZ as the two parameters to toggle (by default, False and True)
  • MRV X/Y/Z and MRV XZ/XYZ as the two parameters to toggle (by default, both False)
  • MWV X/Y/Z and MWV XZ/XYZ as the two parameters to toggle (by default, both False)

scripts/Modules/infodisplay.ini Outdated Show resolved Hide resolved
scripts/Modules/mkw_core.py Outdated Show resolved Hide resolved
scripts/Modules/mkw_core.py Outdated Show resolved Hide resolved
scripts/Modules/mkw_core.py Outdated Show resolved Hide resolved
scripts/Modules/mkw_core.py Outdated Show resolved Hide resolved
scripts/_draw_info_display.py Outdated Show resolved Hide resolved
scripts/_draw_info_display.py Outdated Show resolved Hide resolved
scripts/_draw_info_display.py Outdated Show resolved Hide resolved
scripts/_draw_info_display.py Show resolved Hide resolved
scripts/_draw_info_display.py Outdated Show resolved Hide resolved
@malleoz
Copy link
Collaborator

malleoz commented Sep 28, 2023

Applied all changes from my review to xi's branch. I've limited the digits config so that it will be a max of 7 digits.
Awaiting @vabold opinion on if we should change the time functions to a dataclass still.

@vabold
Copy link
Member

vabold commented Sep 28, 2023

Applied all changes from my review to xi's branch. I've limited the digits config so that it will be a max of 7 digits.
Awaiting @vabold opinion on if we should change the time functions to a dataclass still.

The base game has a timer class, tracking minutes, seconds, milliseconds, and whether or not the timer is active. It's created on the stack frequently, so I'd say it's a good idea to allow the same thing here.

@malleoz
Copy link
Collaborator

malleoz commented Sep 28, 2023

Updated to include a dataclass that abstracts away time manipulation.

@malleoz
Copy link
Collaborator

malleoz commented Sep 30, 2023

Added a commit to address the info display flickering for the frame a savestate is loaded.

scripts/Modules/mkw_classes.py Outdated Show resolved Hide resolved
scripts/Modules/mkw_classes.py Show resolved Hide resolved
scripts/Modules/mkw_core.py Outdated Show resolved Hide resolved
scripts/Modules/mkw_core.py Outdated Show resolved Hide resolved
scripts/Modules/mkw_core.py Show resolved Hide resolved
scripts/Modules/mkw_core.py Outdated Show resolved Hide resolved
@malleoz malleoz requested a review from vabold October 6, 2023 01:31
Copy link
Member

@vabold vabold left a comment

Choose a reason for hiding this comment

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

Backend looks good to me. LGTM assuming _draw_info_display.py is tested and reviewed by someone on the frontend.

@malleoz malleoz merged commit d5939f1 into TASLabz:main Oct 10, 2023
1 check passed
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.

5 participants