citizenfx / fivem

The source code for the Cfx.re modification frameworks, such as FiveM, RedM and LibertyM, as well as FXServer.
https://cfx.re/
3.52k stars 2.07k forks source link

feat(gameinput): new sorting algo and new native #2827

Open ahcenezdh opened 4 days ago

ahcenezdh commented 4 days ago

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

Fixes issues

FabianTerhorst commented 4 days ago

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

tabarra commented 4 days ago

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

ahcenezdh commented 4 days ago

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?

ahcenezdh commented 4 days ago

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

tens0rfl0w commented 4 days ago

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)

ahcenezdh commented 4 days ago

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 commented 4 days ago

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 commented 3 days ago

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.

ahcenezdh commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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.