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

feat: in-game options, flags overhaul #506

Merged
merged 14 commits into from
Oct 29, 2024
Merged

feat: in-game options, flags overhaul #506

merged 14 commits into from
Oct 29, 2024

Conversation

LennardF1989
Copy link
Member

Draft PR with WIP logic for an in-game options menu for Peacock (as a plugin, currently).

Also comes with extended flags.ts to feed the right information to the UI, plus logic to allow plugins to register their own flags (example included).

Added plugin for the in-game menu system
Added example plugin with using custom flags
@RDIL
Copy link
Member

RDIL commented Aug 11, 2024

Lennard I love you this is amazing

@AnthonyFuller
Copy link
Contributor

AnthonyFuller commented Aug 20, 2024

Just pushed a bit of code which "migrates" the old options file. I add the flags we just read under the peacock section. From my testing it seems like it works. It will be for v8 anyway, so breaking changes are allowed.

@LennardF1989
Copy link
Member Author

Just pushed a bit of code which "migrates" the old options file. I add the flags we just read under the peacock section. From my testing it seems like it works. It will be for v8 anyway, so breaking changes are allowed.

Awesome - I mostly did the other options.ini file so I could switch between this and other branches, without constantly losing my stuff. But this is the way forward anyway! Great :)

@AnthonyFuller AnthonyFuller changed the title In-game options menu feat: in-game options, flags overhaul Aug 20, 2024
@AnthonyFuller
Copy link
Contributor

AnthonyFuller commented Aug 20, 2024

My plan for this right now is to override the IOI account portion of the options menu. This will virtually never be removed in an update or changed by mods. It also means it works everywhere. The Peacock tab is cool and all for testing, but moving it into options is better overall.

EDIT: Seems like the IOI account option isn't shown in the pause menu options. We will have to override a template, shame but it has never been changed since chunk0.

@RDIL
Copy link
Member

RDIL commented Aug 21, 2024

what about our evil plans to make everyone require a peacock account to use it /s

@LennardF1989
Copy link
Member Author

LennardF1989 commented Aug 21, 2024

My idea was that plugins could eventually register their own top-level entries in the menu (like the Test one) where they could do whatever they had to for their plugins, not just flags. This could obviously also work under Options :)

On top of that, I think we may need two (or more) cachebuster variables to be able to control when which portion of the menu forcefully updates. In my initial implementation, whenever you do something in the UI you are always kicked back to the root of everything because the root-level also updates. But this may or may not be related to the cachebuster at all, but rather the way the ioiaccount response is handled.

@AnthonyFuller
Copy link
Contributor

Yeah, I think the refresh of the menu is due to the submit-email forcing it.

One way around this is to store all the changes and do a bulk update by either pressing a "Save" button or by doing it when you exit the category.

My idea was that plugins could eventually register their own top-level entries in the menu (like the Test one) where they could do whatever they had to for their plugins, not just flags. This could obviously also work under Options :)

Makes sense, we could still do this within options like you said, plugins could have their own menu with a link to a settings page within it (for flags) and then other custom pages.

@LennardF1989
Copy link
Member Author

Found the issue with the enums, working on a fix.

@LennardF1989
Copy link
Member Author

LennardF1989 commented Aug 21, 2024

The last commit is not in a working state.

  • The index.json tries to look for options.json, but it does not exist.
  • The logic of the old hub.json is not reimplemented, so the peacock:getAllFlags is never executed and thus populated.
  • Hitman crashes upon load if the first issue mentioned is fixed and the second is not.

On top of that, I would really like to have the "Refresh UI" and cache buster data visible when PEACOCK_DEV is set, this really slows down testing changes to the UI. Also a nice excuse to extend the plugin API to be able to add this (along with the old "Test" menu option).

I've pushed my enum fix either way, but this will need additional work.

As for the enums: The problem is context/contextitem vs tabs/tabsitem for the controllers. Because a category can have both another category or flags, enums (as the normal options section uses them) won't work because they require a "tabs" controller. But if you set a category to always be "tabs", it won't be removed from the UI if you go a level deeper. So the best "solution" now was to add enum options as another level, rather than trying to mimic how Hitman usually handles enums.

System is kinda messy, but it works.

@AnthonyFuller
Copy link
Contributor

AnthonyFuller commented Aug 21, 2024

Just pushed options.json, webstorm wasn't tracking it so I didn't realise I hadn't pushed it.

On top of that, I would really like to have the "Refresh UI" and cache buster data visible when PEACOCK_DEV is set, this really slows down testing changes to the UI. Also a nice excuse to extend the plugin API to be able to add this (along with the old "Test" menu option).

Agreed.

System is kinda messy, but it works.

Yeah, menu templates get messy fast. The current system for enums is nice since it still shows the option info on the right, giving you context for what each enum does.

@AnthonyFuller
Copy link
Contributor

What's stopping this from being merged right now?

@RDIL RDIL self-requested a review October 18, 2024 15:24
@LennardF1989
Copy link
Member Author

I can give it a test run this weekend to make sure it's functional and doesn't break backwards compatibility.

@AnthonyFuller
Copy link
Contributor

One thing is that it should be moved into core, not a plugin.

components/flags.ts Outdated Show resolved Hide resolved
components/profileHandler.ts Outdated Show resolved Hide resolved
@LennardF1989
Copy link
Member Author

Update:

  • Moved logic out of plugin to core, exposing hooks and useful functions for plugins
  • Performed the requested code changes

Why this is not ready yet (Must):

  • Menu navigation is annoying, as changing flags kicks you back to the root.
  • Isn't the ispeacockdev.json trick always returning true now, even when you are not running as dev?

Should:

  • Needs documentation for plugin developers, specifically registerFlagSection, addCommandMap and handleCommand-hook.

Nice to have:

  • Needs some additional functions to make it easy to add your own custom menu files. I plan on restoring my original modal, button and forEach test, by only calling some core functions. But this could be a separate ticket.

@LennardF1989 LennardF1989 requested a review from RDIL October 21, 2024 22:32
.substring(peacockCommandPrefix.length)
.split("|")

commands.forEach((c) => {
Copy link
Member Author

@LennardF1989 LennardF1989 Oct 21, 2024

Choose a reason for hiding this comment

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

Should be a const-of, I somehow missed this or accidentally reverted it...


const peacockCommandPrefix = "peacock:"

// LennardF1989: Do we want to consider string[] because of the use of URLSearchParams?
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, no, maybe?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with either, your call

if (command === "clear") {
const commands = (args.commands as string)?.split(",")

for (const c of commands) {
Copy link
Member Author

Choose a reason for hiding this comment

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

c clashes with outer variable, why is the linter fine with this?

@AnthonyFuller
Copy link
Contributor

Menu navigation is annoying, as changing flags kicks you back to the root.

I spoke to RDIL about this, he said that it should just save after every option change (like this). Sure, it's annoying, but it means any changes will be reflected instantly instead of a user forgetting to press save or something.

@LennardF1989
Copy link
Member Author

Menu navigation is annoying, as changing flags kicks you back to the root.

I spoke to RDIL about this, he said that it should just save after every option change (like this). Sure, it's annoying, but it means any changes will be reflected instantly instead of a user forgetting to press save or something.

Agreed, it should immediately change. I was just wondering if there is a way to not reset the menu.

Afaik this happens because the email-endpoint updates the variable, so interesting to see if we can counter that by maintaining a client-side state and not sending back a status-response all the time OR simply remember where the user was in the UI and immediately jump back to that point (similar to what you do with the Freelancer tile that immediately starts it).

But I was out of time yesterday.

@RDIL
Copy link
Member

RDIL commented Oct 29, 2024

Since this is blocking in-game roulette's release, and we don't want users to have to use an unofficial PR branch, going to make the decision to merge this in it's current state. We can do follow-ups to improve it, but I think the current implementation is good enough:tm: to ship in an alpha state.

@RDIL RDIL marked this pull request as ready for review October 29, 2024 23:29
@RDIL RDIL merged commit fa2b9d2 into master Oct 29, 2024
5 checks 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.

3 participants