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(resources-metadata): Set servername convar as global #3008

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ImHoppy
Copy link

@ImHoppy ImHoppy commented Dec 16, 2024

Goal of this PR

This PR serves more as a proof of concept than ready-to-merge code. It introduces functionality to dynamically load files based on a defined ConVar.

For context, our base is compatible with both RDR3 and GTA5, as well as multiple servers. To separate files, we currently use file headers with return conditions (example screenshot).

With this functionality, we can, for instance, load only the files in the gta5 folder when the gamename variable is set to gta5.

Here is an example of a resource that takes advantage of this new functionality:
fxmanifest:

fx_version 'cerulean'
games { 'rdr3', 'gta5' }
version '1.0.0'

if GetConvar('gamename', 'gta5') == 'rdr3' then
    server_script 'redm.lua'
else
    server_script 'fivem.lua'
end

How is this PR achieving the goal

This PR achieves its goal by adding a GetConvar function in the fxmanifest. The function takes two parameters:

  • varName: The name of the ConVar to retrieve.
  • defaultValue: The value to use if the ConVar is not defined.
    Using this function allows conditional logic to determine which files are loaded. This approach simplifies the management of multiple game environments and servers.

This PR applies to the following area(s)

Server on the resources load

Successfully tested on

Custom server build

Game builds: tested on 3095 but should work on any

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

not related to any issues

NOTE: I am fully open to making changes as needed. I am also ready to update the documentation.

@FabianTerhorst
Copy link
Contributor

Maybe just do a method that returns the game name instead of allowing ConVar reads inside the meta data loader?

@ImHoppy
Copy link
Author

ImHoppy commented Dec 16, 2024

That's good too, but ConVar reads let us filter files dynamically per server, like servername == "Unnamed RP".

@ImHoppy
Copy link
Author

ImHoppy commented Dec 16, 2024

But it's true that in reality we only need two variables. And I don't see the use of other variables.

@FabianTerhorst
Copy link
Contributor

Yes, then add the server name and game name as two methods and then it should be fine. Don't really see the need allowing to read any existing ConVar here.

@github-actions github-actions bot added the invalid Requires changes before it's considered valid and can be (re)triaged label Dec 16, 2024
@ImHoppy ImHoppy changed the title feat(resources-metadata): Add GetConvar function feat(resources-metadata): Set servername convar as global Dec 16, 2024
@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 Dec 16, 2024
@pedr0fontoura
Copy link
Contributor

If .cfg is still the recommended way of defining server config, reading ConVars could be relevant.

To illustrate the use case, imagine a resource with an export API and some UI. If the UI is optional, you could use a ConVar to avoid loading all the UI-related files.

@prikolium-cfx
Copy link
Collaborator

I would not recommend to build any logic in fxmanifest.lua. At some point we plan to move to declarative fxmanifest and it means that you won't be able to execute code there.

@ImHoppy
Copy link
Author

ImHoppy commented Dec 17, 2024

And it's not possible to have this pr until you release the v3 of the fxmanifest?

@prikolium-cfx
Copy link
Collaborator

prikolium-cfx commented Dec 17, 2024

And it's not possible to have this pr until you release the v3 of the fxmanifest?

For sure it's possible. But probably it's possible to satisfy your need in more declarative way. For example you can add optional argument to server_script function and pass game name to it or something like this. In that case this feature can be transferred to the next fxmanifest version

So it will look like

server_script 'redm.lua' 'rdr3'
server_script 'fivem.lua' 'gta5'

@FabianTerhorst
Copy link
Contributor

Wouldn't it be better to split it to two resources at that point? One for rdr and one for gta.

@ImHoppy
Copy link
Author

ImHoppy commented Dec 17, 2024

Unfortunately, this isn't a solution for us, splitting the code would force us to maintain the same code twice. To illustrate, here's a breakdown of the line count for our single resource. And excluding GTA5, we still have 160,643 lines of code:

Language Files Blank Comment Code
Lua 1277 31,242 4,737 258,310
SQL 14 248 256 1,566
Total 1291 31,490 4,993 259,876

But the idea from Prikolium also seems like a good one, especially if it seems more coherent for the next version

@tabarra
Copy link
Contributor

tabarra commented Dec 28, 2024

I'm big time against needing to execute lua to get the resource metadata, which might be fine for fxserver, but any other codebase (such as txAdmin or Cfx backends) that wants to read it properly will then need to evaluate lua code. But the need for game-specific definitions is valid.

In monitor/txAdmin, I also want to be able to have different manifest directives depending on the game name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs a preliminary assessment to determine the urgency and required action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants