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

Rework mode select menu #624

Merged
merged 42 commits into from
Sep 10, 2024
Merged

Rework mode select menu #624

merged 42 commits into from
Sep 10, 2024

Conversation

F1F7Y
Copy link
Member

@F1F7Y F1F7Y commented Apr 22, 2023

image

TODO:

  • Add filtering
  • Agree on a way of sorting modes
  • Allow custom modes to set their own category
  • Localization

Docs:

Exposes NSSetModeCategory( string mode, int category ) to allow mods / gamemodes to modify their category.

Categories are difend in a global enum eModeMenuModeCategory

Adds these vanilla files:

  • resource/ui/menus/mode_select.menu

@F1F7Y F1F7Y marked this pull request as draft April 22, 2023 23:28
@F1F7Y F1F7Y marked this pull request as ready for review April 23, 2023 11:41
@F1F7Y
Copy link
Member Author

F1F7Y commented Apr 23, 2023

Marked as ready for review. Would also like to add keyboard navigation, but due to limited time that'll have to wait for next weekend.

@F1F7Y F1F7Y added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Apr 23, 2023
@F1F7Y F1F7Y requested a review from uniboi April 23, 2023 12:01
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks so much for looking into it <3

Playing around with it real quick and this what I noted down:

  • Marked For Death is teambased so it should probably be in PvP and not FFA.
  • Right now categories are sorted alphabetically which is fine but it might make sense to sort them by popularity (not sure about that one though).
  • The category filter doesn't seem to work for me. Switching through the categories it still displays all of them. Not sure what's going on there ^^"

@F1F7Y
Copy link
Member Author

F1F7Y commented Apr 23, 2023

Awesome work, thanks so much for looking into it <3

<3

Right now categories are sorted alphabetically which is fine but it might make sense to sort them by popularity (not sure about that one though).

With mods adding their own gamemodes this is basically impossible. Sorting alphabetically also means some modes may get more exposure

The category filter doesn't seem to work for me. Switching through the categories it still displays all of them. Not sure what's going on there ^^"

Looks like i broke it

@F1F7Y
Copy link
Member Author

F1F7Y commented Apr 23, 2023

Might also be worth removing the Custom category as it doesnt really fit in the PvP, PvE, ... sorting methodology

@GeckoEidechse
Copy link
Member

Might also be worth removing the Custom category as it doesnt really fit in the PvP, PvE, ... sorting methodology

Hmm, maybe. It would be kinda nice to have a distinction between Vanilla, Northstar, and 3rd party mod modes but I'm not sure how to combine that with sorting into PvP, PvE, etc categories without it turning out as a bunch of categories that have like 1-2 entries... ^^"

@GeckoEidechse GeckoEidechse removed the READY TO MERGE This mergeable right now label Aug 22, 2024
@GeckoEidechse
Copy link
Member

Removed "READY TO MERGE" lable until merge conflicts are resolved

@GeckoEidechse GeckoEidechse removed their request for review August 29, 2024 12:31
@GeckoEidechse

This comment was marked as outdated.

@github-actions github-actions bot removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 8, 2024
@GeckoEidechse GeckoEidechse self-assigned this Sep 9, 2024
@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 9, 2024
@github-actions github-actions bot removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 9, 2024
@GeckoEidechse GeckoEidechse added needs testing Changes from the PR still need to be tested almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed commits vanilla file For PRs that commit vanilla files from VPKs labels Sep 9, 2024
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Changes confirmed working in testing.

image

List is nicer than the previous vanilla one. As for the exact ordering I'd say we decide on that post-merge ^^

(Not marking as approved until I got the file diff figured out mentioned in the comment above. After that it's ready to merge :D)

@GeckoEidechse GeckoEidechse removed the needs testing Changes from the PR still need to be tested label Sep 10, 2024
@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 10, 2024
Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Fixed line-endings on main. This should be good to merge now ^^

@GeckoEidechse GeckoEidechse merged commit 525f671 into R2Northstar:main Sep 10, 2024
3 checks passed
@GeckoEidechse
Copy link
Member

Welp so somehow I missed that it kinda just marks all gamemodes as locked, fuck

I really shouldn't test PRs while exhausted x_x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge merge conflicts Blocked by merge conflicts, waiting on the author to resolve
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants