-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Migrate NPC rules to a new, dedicated imgui menu #74359
Conversation
6109f4e
to
56e356a
Compare
clicking the x on the menu makes it unresponsive until re-opened (also no indication to hit esc or space for closing it, in general) |
I am aware, thank you. |
"Hm I don't know how to solve this, how did I do it on the mission UI?"
THANKS ME |
I'm marking this ready for review, because that's what it is: Ready for review. I am reasonably confident in it, though I may still push more commits. There was a reasonable request to switching from using "CHANGE" buttons to checkboxes. There's also some boilerplate which could stand to be templated. |
3933854
to
e24480b
Compare
…by pressing close button. Also fixed this in mission ui
e24480b
to
2fd22db
Compare
2fd22db
to
97a3aa1
Compare
Instead of checkboxes, we got colored buttons. (Checkboxes could still be added later, but colorizing the buttons keeps it consistent and means I don't have to do anything else!) Template was implemented to keep nice and streamlined. The only thing I could want now is a confirmation on defaulting all settings. I was using query_yn for this, but it causes a crash(see code comments). Assuming tests pass, we are good to go. |
9f71e54
to
6e43d5a
Compare
a1afc7e
to
567b509
Compare
aa3ea01
to
e5c7dd0
Compare
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.
I think that the boolean options should be checkboxes, and the multiple–choice ones should be list boxes or combo boxes.
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.
And the same for all the other repetitions of the same pattern in the rest of the code.
Another thing, the import/export dialog is really confusing if you only have one NPC, because the table is empty. |
...Is it? It ends up being an empty table, which makes sense to me. |
Normally one exports things to a file, so it made no sense until I went back and read the source code. Maybe the wording should be changed, and perhaps there should be some acknowledgement that you haven’t got any other NPCs to make it more clear that you should return to this window later. |
1e8b715
to
4efdf4b
Compare
-Extract the common button setup into a function -Only pop style color when it's been pushed -Export/Import displays warning when only 1 follower -other very minor changes
4efdf4b
to
67de86f
Compare
|
Yea, that works. I think it would also be better to avoid the terms “import” and “export”. Perhaps “copy” instead? Also, is there really a need for two buttons as well as a toggle inside the transfer window itself? I think that a single button labeled “Copy rules to/from other followers” would suffice. |
(tl;dr: yes send PR thank you!) I don't think it'd be wrong, I just... prefer? the colored buttons. Basically, -I like the colored buttons. (100% subjective opinion) -The colored buttons offers a more CDDA-style look (again, 100% subjective opinion) -I do not think checkboxes would convey what is happening in the import/export screen, which means regardless the button implementation stays there. Points in favor of checkboxes: -Your checkbox implementation looks pretty sexy -If you already have the branch, any cost of switching to it has already been paid So if it's not any trouble to you, please send the PR . Or PR directly to cleverraven if/after this one is merged, if that would be easier for you.
It previously used I was trying to keep the strings as close to the old ones as possible, but might be worth kicking that one to something different. |
Did you by any chance change your github preferences so that you don’t get pull requests in your fork? It doesn’t give me the option of sending you one. You could also just pull from my branch and push it to yours, if that’s easier than mucking about with github. |
I don't believe I've made any changes, and just recently got the first and only PR to my fork. But sure, you can just link the branch. I can merge, cherry pick, etc as appropriate |
BTW, I did manage to make a PR the other day: RenechCDDA#2 |
This looks really cool! |
There’s conflicts to resolve if this is ready for merge? |
I need to rebase it (to keep the commit history clean-ish) and then check out db48x's branch, I just haven't gotten around to it despite constantly expecting to |
I’m not busy today so I’ll go ahead and rebase for you. I’ll end up with a new PR though. |
Well I guess that WOULD let me work on something else... |
Summary
Interface "NPC follower rules can now be viewed and modified in a dedicated, mouse-supported window (imgui)"
Purpose of change
Everyone agrees the dialogue menu does its job for this, but it could be so much better...
Describe the solution
Make it better, and with making it better comes mouse support!
There is now a dedicated top-level talk topic which opens this new imgui window, no need to remember which of the sub-menus your specific rule was in. Each rule can be fully adjusted with a dedicated hotkey or mouse-sensitive buttons. For rules with multiple states (e.g. CBM recharge %) keyboard presses of the hotkey will cycle to the next setting, while mouse presses are set directly to the selected % button. Keyboard users also have the option of navigating(standard up/down/left/right inputs) directly to the desired setting and press enter, instantly setting that value without a need to toggle through the proceeding ones.
(Still WIP) Tie the imgui buttons to a specific set of auto-assigned hotkeys, like our dialogue does. This is almost certainly a requirement before this PR can be undrafted.Describe alternatives you've considered
Testing
2024-06-08.09-05-26.mp4
Additional context
In the interests of encouraging reviews, I have added a hopefully funny easter egg somewhere in this PR. It will only appear in game with a 0.1% chance.