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

Add meta.xml loading files pattern #3203

Merged
merged 10 commits into from
May 23, 2024

Conversation

W3lac3
Copy link
Contributor

@W3lac3 W3lac3 commented Oct 10, 2023

Fixes #2200

This PR makes it easier to load files from a resource. In order to improve the lives of script developers, it adds a pattern using a glob library.

Some tests have been done and everything seems to be working normally, so if any changes are needed, I'll be making them.

Example of use:

<meta>
    <script src="shared/**/*.lua" type="shared" cache="false" />
    <script src="client/**/*.lua" type="client" cache="false" />
    <script src="server/**/*.lua" type="server" />

    <file src="assets/fonts/**/*.ttf" />
    <file src="assets/sounds/**/*.mp3" />
    <file src="assets/images/**/*.png" />
</meta>

For more information on the library used, click here.

@Allerek
Copy link
Contributor

Allerek commented Oct 10, 2023

Fixes #2200

This PR makes it easier to load files from a resource. In order to improve the lives of script developers, it adds a pattern using a glob library.

Some tests have been done and everything seems to be working normally, so if any changes are needed, I'll be making them.

Example of use:

<meta>
    <script src="shared/**/*.lua" type="shared" cache="false" />
    <script src="client/**/*.lua" type="client" cache="false" />
    <script src="server/**/*.lua" type="server" />

    <file src="assets/fonts/**/*.ttf" />
    <file src="assets/sounds/**/*.mp3" />
    <file src="assets/images/**/*.png" />
</meta>

Add exclude option, to mark files that should not be loaded

<exclude src=" " />

@W3lac3
Copy link
Contributor Author

W3lac3 commented Oct 11, 2023

Fixes #2200
This PR makes it easier to load files from a resource. In order to improve the lives of script developers, it adds a pattern using a glob library.
Some tests have been done and everything seems to be working normally, so if any changes are needed, I'll be making them.
Example of use:

<meta>
    <script src="shared/**/*.lua" type="shared" cache="false" />
    <script src="client/**/*.lua" type="client" cache="false" />
    <script src="server/**/*.lua" type="server" />

    <file src="assets/fonts/**/*.ttf" />
    <file src="assets/sounds/**/*.mp3" />
    <file src="assets/images/**/*.png" />
</meta>

Add exclude option, to mark files that should not be loaded

<exclude src=" " />

Right, but what would be the need for that? I say this because it could just load the file in isolation, doing it this way:

<meta>
    <script src="shared/main.lua" type="shared" cache="false" /> <!-- single file -->
    <script src="client/**/*.lua" type="client" cache="false" />
</meta>

@androksi
Copy link
Member

Finally we will no longer need some external software or write/copy/paste 1000 lines of assets.

@Allerek
Copy link
Contributor

Allerek commented Oct 18, 2023

Fixes #2200
This PR makes it easier to load files from a resource. In order to improve the lives of script developers, it adds a pattern using a glob library.
Some tests have been done and everything seems to be working normally, so if any changes are needed, I'll be making them.
Example of use:

<meta>
    <script src="shared/**/*.lua" type="shared" cache="false" />
    <script src="client/**/*.lua" type="client" cache="false" />
    <script src="server/**/*.lua" type="server" />

    <file src="assets/fonts/**/*.ttf" />
    <file src="assets/sounds/**/*.mp3" />
    <file src="assets/images/**/*.png" />
</meta>

Add exclude option, to mark files that should not be loaded
<exclude src=" " />

Right, but what would be the need for that? I say this because it could just load the file in isolation, doing it this way:

<meta>
    <script src="shared/main.lua" type="shared" cache="false" /> <!-- single file -->
    <script src="client/**/*.lua" type="client" cache="false" />
</meta>

You might have some files in your gamemode that should not be loaded(eg. archival files/old files) - remember that MTA have deprectation upgrade system that renames old files to something like [filename]_old.lua, and we do not want them to be loaded

@dmi7ry
Copy link

dmi7ry commented Oct 18, 2023

Just in case. If something like *.lua used, does it include on the client only those files that are on the server? Or will all files from the client's folder be included?

@W3lac3
Copy link
Contributor Author

W3lac3 commented Oct 18, 2023

Just in case. If something like *.lua used, does it include on the client only those files that are on the server? Or will all files from the client's folder be included?

The whole system is made by the server, so it doesn't load files from the client, only those that are actually in the resource folder.

@W3lac3
Copy link
Contributor Author

W3lac3 commented Oct 18, 2023

You might have some files in your gamemode that should not be loaded(eg. archival files/old files) - remember that MTA have deprectation upgrade system that renames old files to something like [filename]_old.lua, and we do not want them to be loaded

Regarding the MTA update system for deprecated functions, it changes the extension of the file placed like this fileName.lua.old. Since it changes the extension, it won't load it, but I understand your point, but if there really are files that shouldn't be loaded or that are no longer used, I don't see why they should be in the folder that will load them automatically. I would see a problem if MTA renamed the file names to something like fileName_old.lua, because then it would be loaded, but that doesn't happen, so it wouldn't be a problem for the developer.

Something I forgot to put in the description of the PR was the library used, but I've already put it in the description, so there you'll have more information about what it's really possible to do with this feature I'm trying to bring to MTA.

@TracerDS
Copy link
Contributor

TracerDS commented Oct 19, 2023

Does your glob work with regex too or only with * as in bash?

@W3lac3
Copy link
Contributor Author

W3lac3 commented Oct 19, 2023

Does your glob work with regex too or only with * as in bash?

Take a look at the repository of the library used, there you will see what you can do. Just click here to go to the repository. But to answer your question, there are other wildcards besides *.

Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
@Allerek
Copy link
Contributor

Allerek commented Oct 19, 2023

Just in case. If something like *.lua used, does it include on the client only those files that are on the server? Or will all files from the client's folder be included?

The whole system is made by the server, so it doesn't load files from the client, only those that are actually in the resource folder.

You should apply that for client too as its mainly client resources that have many files(GUI elements[images], models etc) that are pain in the A to be inserted into meta.

@W3lac3
Copy link
Contributor Author

W3lac3 commented Oct 19, 2023

Just in case. If something like *.lua used, does it include on the client only those files that are on the server? Or will all files from the client's folder be included?

The whole system is made by the server, so it doesn't load files from the client, only those that are actually in the resource folder.

You should apply that for client too as its mainly client resources that have many files(GUI elements[images], models etc) that are pain in the A to be inserted into meta.

Files such as images and the like are loaded. What I mean is that it doesn't load the files from the player's computer, only the files that are on the server.

@W3lac3 W3lac3 requested a review from tederis October 24, 2023 11:26
Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CResource.cpp Show resolved Hide resolved
Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
Server/mods/deathmatch/logic/CResource.cpp Outdated Show resolved Hide resolved
@tederis
Copy link
Collaborator

tederis commented Dec 10, 2023

LGTM

@StrixG StrixG added the enhancement New feature or request label Dec 21, 2023
@AlexTMjugador
Copy link
Member

AlexTMjugador commented Jan 4, 2024

In #2200 (comment), @sbx320 asked about the order in which files matched by globs are loaded. As far as I can see, that question was not answered on this PR.

Currently, as far as I can see from the glob library source, the order depends on how the operating system enumerates files in directories, which may not be constant across environments. But since globbed files are inert data blobs and not Lua code, I don't see how different loading orders could matter, as long as files are available before any Lua code gets to execute. I think some assurance that my hypothesis is correct would be useful.

Other than that, this PR LGTM, nice job! 🚀

Edit: I asked @W3lac3 about the ordering concerns on Discord, this was their reply:
W3lac3's reply to file enumeration order concerns

@Pirulax
Copy link
Contributor

Pirulax commented Jan 5, 2024

Could add a -- Priority: (Or something similar) to the top of the Lua file.
Though, that won't work in case of precompiled files.

I guess if the order of something is important, it can always be loaded explicitly, no?

@sbx320
Copy link
Member

sbx320 commented Jan 5, 2024

I'd prefer some form of default order (alphabetically?). Otherwise one might rely on the resource working on Windows only to run into weird errors when using a Linux server later.

If another order is required, the explicit loading is fine. I just want us to have consistent behavior between servers, regardless of OS.

@W3lac3
Copy link
Contributor Author

W3lac3 commented Jan 5, 2024

Could add a -- Priority: (Or something similar) to the top of the Lua file. Though, that won't work in case of precompiled files.

I guess if the order of something is important, it can always be loaded explicitly, no?

So, referring to something that must be loaded before the others. It can be loaded separately, as I said with regard to useful functions, basically what I mean by that would be this:

<meta>
    <script src="client/lib/*.lua" type="client" cache="false" /> <!-- This will be loaded first -->
    <script src="client/*.lua" type="client" cache="false" /> <!-- Load all scripts in the client folder -->
</meta>

@Allerek
Copy link
Contributor

Allerek commented Jan 5, 2024

I still feel like there should be way to exclude files from loading.

@W3lac3
Copy link
Contributor Author

W3lac3 commented Jan 5, 2024

I'd prefer some form of default order (alphabetically?). Otherwise one might rely on the resource working on Windows only to run into weird errors when using a Linux server later.

If another order is required, the explicit loading is fine. I just want us to have consistent behavior between servers, regardless of OS.

I hadn't looked more clearly at how the list returns in the library I used, with that in mind. I decided to do some tests using Windows and another in a Linux environment. As far as I can see, the behavior of both is the same, it doesn't change. If you use the default to pick up all files, even those in another folder and so on. The first folder will always be read alphabetically, then the other folders contained in this folder will be read, also following the folder names, in alphabetical order until it finds all the files following the pattern provided.

Here's a screenshot I took of the terminal running on Linux and another on Windows. I've set the code to print a message when they are loaded.

Captura de tela 2024-01-05 141936

Folder organization:

image

@myusernamewo
Copy link
Contributor

In my view, I consider this to be one of the most efficient features ever created, especially in the area of ​​file uploading. In fact, there is no problem with the code and should be incorporated into the game to allow other developers to improve the experience in the .xml file extension.

@Disinterpreter
Copy link
Member

Disinterpreter commented May 1, 2024

Could add a -- Priority: (Or something similar) to the top of the Lua file. Though, that won't work in case of precompiled files.

I guess if the order of something is important, it can always be loaded explicitly, no?

Idk, in Unix OS that problem was solved by name like 00file.conf 01anotherfile.conf
https://unix.stackexchange.com/a/81723

If even OS do this, I didn't see the problem make the same.

@Fernando-A-Rocha
Copy link
Contributor

LGTM. Every MTA developer will appreciate this enhancement.

@Rilot06
Copy link

Rilot06 commented May 5, 2024

It doesn't look merge ready to me. The ideal way to make use of this change would be to make a universal meta.xml file for every single resource you make with scripts and asset file paths predefined so you wouldn't have to change anything in the meta file again. Currently this isn't possible with this, because if a script/file tag is added where the server can't match any files to the src glob, the resource won't start due to an error. The solution would be to either make that error message a warning only or completely remove it. With glob matching I don't see a reason for a resource to not start if the glob can't match any file.

@Fernando-A-Rocha
Copy link
Contributor

Fernando-A-Rocha commented May 5, 2024

With glob matching I don't see a reason for a resource to not start if the glob can't match any file.

Agree, make it a warning

Edit; On second thought, the warnings would be annoying. It should just ignore not outputting any message.

@TracerDS
Copy link
Contributor

TracerDS commented May 5, 2024

I'm opposed to having warnings with patterns. If the pattern didnt match anything it should just ignore the results and not include anything.
Direct include (<script src='server.lua'/>) it should either throw a warning or an error (nothing changes)

@Rilot06
Copy link

Rilot06 commented May 5, 2024

I'm opposed to having warnings with patterns. If the pattern didnt match anything it should just ignore the results and not include anything. Direct include (<script src='server.lua'/>) it should either throw a warning or an error (nothing changes)

Yep, exactly, glob patterns should ignore it completely and I think implicit includes should only throw warning, not error, like how it currently is.

@W3lac3
Copy link
Contributor Author

W3lac3 commented May 5, 2024

I'm opposed to having warnings with patterns. If the pattern didnt match anything it should just ignore the results and not include anything. Direct include (<script src='server.lua'/>) it should either throw a warning or an error (nothing changes)

This has already been discussed in the Discord group. It was decided to keep the code like this and then, after accepting this PR, to open another PR to make these changes.

@Rilot06
Copy link

Rilot06 commented May 5, 2024

I'm opposed to having warnings with patterns. If the pattern didnt match anything it should just ignore the results and not include anything. Direct include (<script src='server.lua'/>) it should either throw a warning or an error (nothing changes)

This has already been discussed in the Discord group. It was decided to keep the code like this and then, after accepting this PR, to open another PR to make these changes.

It was "decided" by one person's reply against multiple who said it's somewhat useless this way. Nightly builds are still down, the PR has been open for months, I don't see why a quick discussion and a quick fix after is that hard..

@W3lac3
Copy link
Contributor Author

W3lac3 commented May 6, 2024

It was "decided" by one person's reply against multiple who said it's somewhat useless this way. Nightly builds are still down, the PR has been open for months, I don't see why a quick discussion and a quick fix after is that hard..

I can make this change, as I said in the Discord group, I also agree that it should work this way. However, I kept the integrity of the current code to have a better chance of being accepted. However, as you said yourself, this PR has been open for a long time, and I'm afraid it will take even longer if I open this discussion. So I'll only do it if the majority agrees with such a change.

@Rilot06
Copy link

Rilot06 commented May 7, 2024

I can make this change, as I said in the Discord group, I also agree that it should work this way. However, I kept the integrity of the current code to have a better chance of being accepted. However, as you said yourself, this PR has been open for a long time, and I'm afraid it will take even longer if I open this discussion. So I'll only do it if the majority agrees with such a change.

Well 2 people agreed with it here, multiple other people agreed with it on discord in that small discussion, nobody disagreed, I see that as majority of people who voice their opinions about it, but you do you. Btw it was already said that the PR will be merged some time after nightly builds are working again, so I don't see how this change would make the PR unmerged for longer time. Anyways, I'll wait and see what happens, if it stays like this, I simply won't use it, since then it's basically pointless to me.

@W3lac3
Copy link
Contributor Author

W3lac3 commented May 9, 2024

Well 2 people agreed with it here, multiple other people agreed with it on discord in that small discussion, nobody disagreed, I see that as majority of people who voice their opinions about it, but you do you. Btw it was already said that the PR will be merged some time after nightly builds are working again, so I don't see how this change would make the PR unmerged for longer time. Anyways, I'll wait and see what happens, if it stays like this, I simply won't use it, since then it's basically pointless to me.

I've set it to ignore wildcard directories, since I agree with this change and I see that some others do too. I haven't added any warning messages either, as I don't think it's necessary.

Copy link
Member

@Lpsd Lpsd left a comment

Choose a reason for hiding this comment

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

LGTM. Please update https://wiki.multitheftauto.com/wiki/Meta.xml to reflect these changes once the nightly build is up.

Use the "new feature" template if possible (example shown here).

@Lpsd Lpsd merged commit 90e2737 into multitheftauto:master May 23, 2024
6 checks passed
MTABot pushed a commit that referenced this pull request May 23, 2024
90e2737 Add meta.xml loading files pattern (#3203)
56a77fe Update client en_US pot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add <directory src="..."/> to meta.xml