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(gameinput): new sorting algo and new native #2827

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

spacevx
Copy link
Contributor

@spacevx spacevx commented Oct 1, 2024

Goal of this PR

Actually, @tabarra added # in his key mappings to have the txAdmin key mappings at the top of the FiveM key settings. This PR aims to fix this so Tabarra doesn't have to do this workaround

How is this PR achieving the goal

If the left is monitor and not the right one, we return true to indicate that the left needs to be placed before the right. If the right one is txAdmin and not the left one, we return false so that the right is placed before the left. Thanks @FabianTerhorst for fixing the sorting logic.

This PR applies to the following area(s)

FiveM

Successfully tested on

image
Here is a screenshot where i did the test with a resource called ns_carrierheist.

Game builds: 3258

Platforms: Windows,

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Oct 1, 2024
@github-actions github-actions bot added invalid Requires changes before it's considered valid and can be (re)triaged and removed triage Needs a preliminary assessment to determine the urgency and required action labels Oct 1, 2024
@FabianTerhorst
Copy link
Contributor

Maybe ordering by a integer would be more flexible then limiting it to txAdmin itself?

@tabarra
Copy link
Contributor

tabarra commented Oct 1, 2024

Thanks for the PR.
Just as clarification, although I like having it at the top of the list, my only real concern is having all the txAdmin keybinds to show grouped as one block. As I see, ideally the sorting should be based off the resource name, and within the group the sorting can be alphabetical.
But ordering by arbitrary integers would work as well, but in tx I would probably hardcode -9999 or +9999 🤣

And btw, the reason why txAdmin has ui_label 'txAdmin' in the fxmanifest.lua is because it was the plan to have it be used for the renaming of resources in the keybinds ui (monitor -> txAdmin), it would also be useful for some big resources like es_extended -> ESX

@spacevx
Copy link
Contributor Author

spacevx commented Oct 1, 2024

Thanks for the PR. Just as clarification, although I like having it at the top of the list, my only real concern is having all the txAdmin keybinds to show grouped as one block. As I see, ideally the sorting should be based off the resource name, and within the group the sorting can be alphabetical. But ordering by arbitrary integers would work as well, but in tx I would probably hardcode -9999 or +9999 🤣

And btw, the reason why txAdmin has ui_label 'txAdmin' in the fxmanifest.lua is because it was the plan to have it be used for the renaming of resources in the keybinds ui (monitor -> txAdmin), it would also be useful for some big resources like es_extended -> ESX

Good idea. So @FabianTerhorst wants me to do an order system when calling RegisterKeyMapping. You can pass an integer; the larger the number, the higher the key map is at the top in terms of priority. If some resources are using the same order, alphabetic sorting will apply. We're also going to group key maps from the same resource together. Is it good for you @FabianTerhorst?

@spacevx
Copy link
Contributor Author

spacevx commented Oct 1, 2024

And for the ui_label thing i think it's better to hide them with a native i don't understand why names are here

@spacevx spacevx marked this pull request as draft October 1, 2024 19:50
@tens0rfl0w
Copy link
Contributor

tens0rfl0w commented Oct 1, 2024

The resource name is there to provide "persistence" as keybinds per resource exist across different servers.

Originally, the resource name was behind the actual keybind name, which ofc makes more sense, but IIRC this was changed because resource developers were able to hide the resource name with some "invalid" Scaleform characters.

(I think allowing resources to define their keybind priority can get quite messy and lacks any deterministic rule, so not sure if implementing this for any resource is a good idea)

@spacevx
Copy link
Contributor Author

spacevx commented Oct 2, 2024

The resource name is there to provide "persistence" as keybinds per resource exist across different servers.

Originally, the resource name was behind the actual keybind name, which ofc makes more sense, but IIRC this was changed because resource developers were able to hide the resource name with some "invalid" Scaleform characters.

(I think allowing resources to define their keybind priority can get quite messy and lacks any deterministic rule, so not sure if implementing this for any resource is a good idea)

Yeah but server owners can easily change the keybinds, i think a function to disable/enable name of resources in the key page would be cool, and yeah for the sorting can be messy

@tabarra
Copy link
Contributor

tabarra commented Oct 2, 2024

I feel like maybe this is getting derailed or out of scope.
At least for txAdmin's case, just grouping all txAdmin keybinds together is enough for me to remove the silly # prefix that I added, even if the groups themselves end up being sorted alphabetically without any customization option.

@Bossman02
Copy link

I think that the grouping of keybinds by resource is something interesting to happen, since the fact that resources with many assignments currently have them spread throughout the list, sometimes makes it difficult to locate which one you want to edit, as each server has its own list of resources making the positions change (I would even say that the order changes every time you connect, but I'm not entirely sure).

From my point of view the best thing would be to sort alphabetically first by resource and then by keybind description and if a custom sorting is allowed, that what can be changed is the position of the whole group and that it is at server configuration level, not by decision of the resource creator.

@github-actions github-actions bot added triage Needs a preliminary assessment to determine the urgency and required action and removed invalid Requires changes before it's considered valid and can be (re)triaged labels Oct 4, 2024
@spacevx spacevx changed the title feat(gameinput): put txAdmin keys mappings at the top feat(gameinput): new sorting algo and new native Oct 4, 2024
@spacevx
Copy link
Contributor Author

spacevx commented Oct 4, 2024

Ok, so now the sorting algorithm is based on the resource names and then alphabetically within each resource a native was added SET_KEY_MAPPING_HIDE (if you have a better name don't hesitate) which allows hiding resource names from the FiveM keys page

Here is a render of the key page without resource names
image

And another for the sorting
image

@Bossman02
Copy link

I do not think that hiding the names of the resources is a good option, there may be several that have the same descriptions in their actions (a clear example is the different libraries that usually have their “interact” action).

@FabianTerhorst
Copy link
Contributor

Ok, so now the sorting algorithm is based on the resource names and then alphabetically within each resource a native was added SET_KEY_MAPPING_HIDE (if you have a better name don't hesitate) which allows hiding resource names from the FiveM keys page

Here is a render of the key page without resource names image

And another for the sorting image

Maybe name it SET_KEY_MAPPING_HIDE_RESOURCE.

@FabianTerhorst
Copy link
Contributor

I do not think that hiding the names of the resources is a good option, there may be several that have the same descriptions in their actions (a clear example is the different libraries that usually have their “interact” action).

Hiding the names is fine in my opinion. The server can decide and the user don't know what "ns__carrierheist" in front of the key means.

@spacevx spacevx marked this pull request as ready for review October 5, 2024 17:09
* Added a native to be able to hide all resource names from the FiveM key page
* Removed specific handling for txAdmin
* Improved sorting algorithm to sort by resource names and then alphabetically within each resource (including lowercase)
@github-actions github-actions bot added invalid Requires changes before it's considered valid and can be (re)triaged and removed triage Needs a preliminary assessment to determine the urgency and required action labels Oct 5, 2024
tabarra added a commit to tabarra/txAdmin that referenced this pull request Oct 11, 2024
@martonp96
Copy link

Tested, working.
Thank you for the contribution.

@martonp96 martonp96 self-requested a review October 21, 2024 14:31
@martonp96 martonp96 added ready-to-merge This PR is enqueued for merging and removed invalid Requires changes before it's considered valid and can be (re)triaged labels Oct 21, 2024
@prikolium-cfx prikolium-cfx merged commit 1e208ce into citizenfx:master Oct 28, 2024
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR is enqueued for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants