-
Notifications
You must be signed in to change notification settings - Fork 630
Integrate Randomizer Window into Modern Menu Properly #6017
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
Integrate Randomizer Window into Modern Menu Properly #6017
Conversation
Not fully functional, but actually builds and renders correctly. Still need to split up UpdateOptionProperties and assign callbacks and also set up a CustomWidget for Excluded Locations and Tricks.
|
Was the |
|
This is a branch that I started on months ago, stopped for a while, and then resurrected, so I'm not 100% sure exactly what was happening that would've prompted me to add it. I'm fairly certain that if it's in there it's because I ran into an issue that it resolved, but my memory on that is hazy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finally freestanding item shuffle not out of place. things were getting crowded with all the new shuffles
imo we should replace #ifdef guards with #pragma once, LUS already has it happening
|
Headerguards removed, it built fine locally, I guess I didn't need them after all, not sure why I added them. |
|
Addressed review comments from @Pepe20129 |
Pepe20129
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small non-blocking thing.
|
Do we care about rando settings showing up in search normally? |
|
I kind of assumed that would have been a desired behavior, is it not? |
|
I'm conflicted, personally. On one hand, it just works as it is, but on the other, there are so many rando options, having them clutter the search when people are looking for enhancements doesn't seem like good UX. |
|
Hmmm, maybe search filtering options are in order beyond just filtering out these, maybe we should be implementing more generic filters so in addition to filtering out rando options they can also filter out say, cheats, or something else like that. I can definitely see people wanting to search specifically for a rando option as well. |
|
Well, I do think that general filters probably would be a good thing, though I'm not sure how we could implement them seamlessly in a way that would be intuitive for the user. Perhaps a button next to (or inside at the right end) of the search field that would bring up a list of filters that can be applied (maybe, at first, relating to headers or CVar prefixes), that would then put the filter text into the search field so the user could see how they can apply it themselves without the filter window? Up to you whether you want to apply that here, but if not, I think you can just resolve the conflicts and we can get it in as is. Even if the rando options show up in general search, it's still better to have them there than not. |
|
Alright, got it merged up with latest develop, had to do some manual stuff for the added bean stuff, I think I got it all but a sanity check from someone else would be good. |
Also comes with a menu reorganization. Please give feedback on this aspect if you think it can still be organized better!
Also randomizer/context was renamed to SeedContext because I was having name collision issues with the Context in LUS.
Build Artifacts