Add `getResourceFiles` #3603

TracerDS · 2024-07-23T12:23:32Z

getResourceFiles will return names of all files loaded by a resource

Fernando-A-Rocha · 2024-07-24T00:07:14Z

Very nice. Can you add an argument "resource element" so you can get file list of another resource, which would work if it is running?

TracerDS · 2024-07-24T06:40:25Z

Very nice. Can you add an argument "resource element" so you can get file list of another resource, which would work if it is running?

Already did ;)

Server/mods/deathmatch/logic/luadefs/CLuaResourceDefs.cpp

Fernando-A-Rocha

LGTM

Client/mods/deathmatch/logic/luadefs/CLuaResourceDefs.cpp

Server/mods/deathmatch/logic/CResourceFile.h

TheNormalnij · 2024-08-01T15:28:52Z

Client/mods/deathmatch/logic/CDownloadableResource.cpp

+    switch (resourceType)
+    {
+        case eResourceType::RESOURCE_FILE_TYPE_SCRIPT:
+        case eResourceType::RESOURCE_FILE_TYPE_CLIENT_SCRIPT:
+            m_resourceCategoryType = eResourceCategory::SCRIPTS;
+        case eResourceType::RESOURCE_FILE_TYPE_MAP:
+            m_resourceCategoryType = eResourceCategory::MAPS;
+        case eResourceType::RESOURCE_FILE_TYPE_CONFIG:
+        case eResourceType::RESOURCE_FILE_TYPE_CLIENT_CONFIG:
+            m_resourceCategoryType = eResourceCategory::CONFIGS;
+        case eResourceType::RESOURCE_FILE_TYPE_CLIENT_FILE:
+        case eResourceType::RESOURCE_FILE_TYPE_HTML:
+            m_resourceCategoryType = eResourceCategory::FILES;
+    }
+


You can filter it in CLuaResourceDefs::GetResourceFiles. You don't need GetResourceCategoryType

Its better this way

Hmm. You can use original types. I think client-scripts and server-scripts have some sense for developers. So you shouldn't map eResourceType into custom enum for one Lua function

I'd have to do string manipulation in both functions just to get values.
Its better that way

What do you mean?

What is wrong with eResourceType ?

too much noise

It looks like using existing enum is much better than creating a new one with mapping from eResourceType and adding boilerplate code in client and server.

too much noise

What do you mean?

What do you mean?

Exactly what I said. Its easier to have 3 categories: scripts, maps, files instead of: client/server scripts, client/server files, (client/server?) maps, etc.

I see no problem here. It's much useful.

TheNormalnij · 2024-08-03T11:34:45Z

Does it return shared scripts twice here? Is it expected behavior?

local list = getResourceFiles("all")

Fernando-A-Rocha · 2024-08-04T01:20:18Z

File paths returned shouldn't be repeated imo

Fernando-A-Rocha · 2024-08-04T01:21:44Z

To make this more advanced, the list returned could be a table of {filePath, fileType} where fileType is like "client-script" or "shared-script" or "map-file" idk

TracerDS · 2024-08-04T16:27:19Z

Does it return shared scripts twice here? Is it expected behavior?
local list = getResourceFiles("all")

Yeah, it returns twice. Its not expected behaviour. Will fix that

TheNormalnij · 2024-08-04T16:56:35Z

Server/mods/deathmatch/logic/luadefs/CLuaResourceDefs.cpp

+        if (file->GetResourceCategoryType() != type.value() && type.value() != eResourceCategory::ALL)
+            continue;
+        files.insert(file->GetName());


Previous varian was more readable. Why did you change it?

previous variant was basically the same.
This leaves room for more conditions to come.

There is no 'early continue' rule. You obfuscate the code with overusing 'continue' in loops. And you don't really need to use early return for small code blocks.
You did the code less readable and maintainable.

if (file->GetResourceCategoryType() == type.value() || type.value() == eResourceCategory::ALL) files.insert(file->GetName());

Is fine condition, that describes the function logic.

There is early-return which is basically the same but whatever.
I'd rather go with continue rather than indentation

Your continue creates indentation too. Please, keep it simple

There is early-return which is basically the same but whatever.

Not really. Multiple 'continue' conditions can make a loop harder for reading.
There is no goal to use minimal indentation. The code should primarily describe the logic.
You're overusing 'early return'

Again?

No one was interested in this convo for 2 weeks, give it a break.
No contributors were opposed against this so Im leaving it as it is.

I also see the early return overuse here.

No contributors were opposed against this so Im leaving it as it is.

And nobody confirm your position. Don't hide requested changes. Stale conversations should be dismissed by another members.

And nobody confirm your position

Im following the guidelines is'all.

you interpreter these guidelines very differently

Fernando-A-Rocha

Ready to merge imo

Server/mods/deathmatch/logic/CResource.h

Fernando-A-Rocha · 2024-08-25T00:02:38Z

@TracerDS it would be nice to have a way to know if a file has download="false" attribute, so it can be downloaded using downloadFile later
Currently the only way to achieve this is by parsing the meta.xml file, which seems a bit too much

Fernando-A-Rocha · 2024-11-06T12:17:02Z

@TracerDS it would be nice to have a way to know if a file has download="false" attribute, so it can be downloaded using downloadFile later Currently the only way to achieve this is by parsing the meta.xml file, which seems a bit too much

Any ideas?

TracerDS · 2024-11-06T18:42:11Z

@TracerDS it would be nice to have a way to know if a file has download="false" attribute, so it can be downloaded using downloadFile later Currently the only way to achieve this is by parsing the meta.xml file, which seems a bit too much

Any ideas?

maybe a parameter that checks download="false"?

Fernando-A-Rocha · 2024-11-06T19:38:22Z

@TracerDS it would be nice to have a way to know if a file has download="false" attribute, so it can be downloaded using downloadFile later Currently the only way to achieve this is by parsing the meta.xml file, which seems a bit too much

Any ideas?

maybe a parameter that checks download="false"?

Resource files, when they are parsed in the meta.xml, are already identified if they need to be downloaded later by the client.

Fernando-A-Rocha · 2024-11-07T08:27:14Z

It's a matter of returning that download attribute value for each file. The array of file names returned by this new function should contain arrays where you have like
{fileName="myFoder/myFile.dff", download=false}

TracerDS · 2024-11-07T08:50:27Z

It's a matter of returning that download attribute value for each file. The array of file names returned by this new function should contain arrays where you have like
{fileName="myFoder/myFile.dff", download=false}

I think it would be better to return just an array of files and handle the download attribute via a parameter. It would be better for the end user

Fernando-A-Rocha · 2024-11-07T08:57:27Z

It's a matter of returning that download attribute value for each file. The array of file names returned by this new function should contain arrays where you have like
{fileName="myFoder/myFile.dff", download=false}

I think it would be better to return just an array of files and handle the download attribute via a parameter. It would be better for the end user

True, but what parameter exactly?

TracerDS · 2024-11-07T09:05:55Z

True, but what parameter exactly?

Something like isDownloaded or something like that. Or maybe it will receive a table of flags if more param were to come

TracerDS added 2 commits July 23, 2024 14:13

Add getLoadedFiles

4f3acf9

Updated CLuaResourceDefs.cpp

da7617f

tederis added the enhancement New feature or request label Jul 24, 2024

tederis reviewed Jul 24, 2024

View reviewed changes

Server/mods/deathmatch/logic/luadefs/CLuaResourceDefs.cpp Outdated Show resolved Hide resolved

TracerDS added 4 commits July 24, 2024 10:14

Merge branch 'master' into 230724_Add_getLoadedFiles

adde333

Updated code

8289dcb

Merge branch 'master' into 230724_Add_getLoadedFiles

59052f4

Fix MacOS/Linux builds

17e0c76

Fernando-A-Rocha approved these changes Jul 28, 2024

View reviewed changes

TheNormalnij reviewed Jul 28, 2024

View reviewed changes

Client/mods/deathmatch/logic/luadefs/CLuaResourceDefs.cpp Show resolved Hide resolved

TheNormalnij changed the title ~~Add getLoadedFiles~~ Add getResourceFiles Jul 28, 2024

TracerDS added 2 commits July 28, 2024 19:36

Updated code according to reviews

452dde7

Merge branch 'master' into 230724_Add_getLoadedFiles

5e3d05a

FileEX reviewed Jul 31, 2024

View reviewed changes

Server/mods/deathmatch/logic/CResourceFile.h Outdated Show resolved Hide resolved

TracerDS added 3 commits July 31, 2024 16:46

Update CResourceFile.h

6f6a8b3

Merge branch 'master' into 230724_Add_getLoadedFiles

6127d68

Merge branch 'master' into 230724_Add_getLoadedFiles

7b78449

Fernando-A-Rocha approved these changes Aug 1, 2024

View reviewed changes

TheNormalnij reviewed Aug 1, 2024

View reviewed changes

Merge branch 'master' into 230724_Add_getLoadedFiles

eb831e0

TracerDS added 2 commits August 4, 2024 18:49

Update CLuaResourceDefs.cpp

543e6cd

Update CLuaResourceDefs.cpp

df5275e

TheNormalnij reviewed Aug 4, 2024

View reviewed changes

Merge branch 'master' into 230724_Add_getLoadedFiles

0d5e2e9

TracerDS added 2 commits August 20, 2024 13:02

Merge branch 'master' into 230724_Add_getLoadedFiles

859f7f4

Merge branch 'master' into 230724_Add_getLoadedFiles

e794557

Fernando-A-Rocha approved these changes Aug 23, 2024

View reviewed changes

tederis reviewed Aug 23, 2024

View reviewed changes

Server/mods/deathmatch/logic/CResource.h Show resolved Hide resolved

Merge branch 'master' into 230724_Add_getLoadedFiles

979d2ae

Merge branch 'master' into 230724_Add_getLoadedFiles

04a49af

Merge branch 'master' into 230724_Add_getLoadedFiles

a9c3768

Add getResourceFiles #3603

Are you sure you want to change the base?

Add getResourceFiles #3603

Conversation

TracerDS commented Jul 23, 2024 • edited by TheNormalnij Loading

Fernando-A-Rocha commented Jul 24, 2024

TracerDS commented Jul 24, 2024

Fernando-A-Rocha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheNormalnij Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheNormalnij commented Aug 3, 2024 • edited Loading

Fernando-A-Rocha commented Aug 4, 2024

Fernando-A-Rocha commented Aug 4, 2024

TracerDS commented Aug 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fernando-A-Rocha left a comment

Choose a reason for hiding this comment

Fernando-A-Rocha commented Aug 25, 2024

Fernando-A-Rocha commented Nov 6, 2024

TracerDS commented Nov 6, 2024

Fernando-A-Rocha commented Nov 6, 2024 • edited Loading

Fernando-A-Rocha commented Nov 7, 2024

TracerDS commented Nov 7, 2024

Fernando-A-Rocha commented Nov 7, 2024

TracerDS commented Nov 7, 2024

Add `getResourceFiles` #3603

Add `getResourceFiles` #3603

TracerDS commented Jul 23, 2024 •

edited by TheNormalnij

Loading

TheNormalnij Aug 3, 2024 •

edited

Loading

TheNormalnij commented Aug 3, 2024 •

edited

Loading

Fernando-A-Rocha commented Nov 6, 2024 •

edited

Loading