Iwd project type #66

JezuzLizard · 2023-12-23T14:41:12Z

Resolves issue #27 by adding the ability to create an iwd archive automatically using the linker's project system.

diamante0018 · 2023-12-23T14:54:37Z

src/Linker/Linker.cpp

-    "fastfile",
-    "ipak",
-};
+constexpr const char* PROJECT_TYPE_NAMES[static_cast<unsigned>(ProjectType::MAX)]{"none", "fastfile", "ipak", "iwd"};


The C++ overlords propose this specific syntax to cast enum classes back to their underlying type.
Even more so, "unsigned" is the incorrect type for this enum because it's unspecified in the declaration (default should be int type).

Suggested change

constexpr const char* PROJECT_TYPE_NAMES[static_cast<unsigned>(ProjectType::MAX)]{"none", "fastfile", "ipak", "iwd"};

constexpr const char* PROJECT_TYPE_NAMES[static_cast<std::underlying_type_t<ProjectType>>(ProjectType::MAX)]{"none", "fastfile", "ipak", "iwd"};

Very verbose, but it gets the job done. I would have suggested this but this project still uses C++17 so it can't be done

diamante0018 · 2023-12-23T14:55:15Z

src/Linker/Linker.cpp

+
+        if (!iwdWriter->Write())
+        {
+            std::cout << "Writing iwd failed." << std::endl;


diamante0018 · 2023-12-23T14:57:04Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+    explicit IWDWriterImpl(const std::string& iwdFileName, ISearchPath* assetSearchPath)
+        : m_filename(iwdFileName),
+          m_asset_search_path(assetSearchPath),
+          m_files(0)


This is the spookiest way I've seen to initialize a vector container. It's hard to read, I would have expected the field to be an integer but it's a vector!

Changed to use the clear() method.

You should not need to explicitly initialize a vector. If you just omit any initialization it will call the default constructor of std::vector which does properly initialize it as empty vector

diamante0018 · 2023-12-23T14:57:39Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+          m_asset_search_path(assetSearchPath),
+          m_files(0)
+    {
+        m_zipfile = zipOpen(m_filename.c_str(), APPEND_STATUS_CREATE);


Are you sure you want to append? What if it already exists? Files duplicate?

As far as i understood it, this does make exactly what you are expecting: It creates the file instead of appending.

The APPEND_STATUS_ is just the prefix and the CREATE is the actual chosen value.

Compared to the source:

#define APPEND_STATUS_CREATE (0) #define APPEND_STATUS_CREATEAFTER (1) #define APPEND_STATUS_ADDINZIP (2)

diamante0018 · 2023-12-23T14:58:46Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+
+    void AddFile(std::string fileName) override
+    {
+        m_files.emplace_back(std::move(fileName));


Unnecessary std::move, you are misusing either emplace_back or std::move.
The best thing to do would be to remove std::move

This is not true. The std::move does make the emplace_back use the move constructor. That is necessary because calling the method itself does make the copy already since it's not a reference.

So it's correct to either use std::move here or otherwise make the parameter a const std::string& and use no std::move

diamante0018 · 2023-12-23T15:01:09Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+
+    bool Write() override
+    {
+        const auto result = std::all_of(m_files.begin(),


Could use cbeing and cend in this case I think. No functional change

I mean what's wrong with it. I think i used all_of a fair bit in the code 😅

No, I mean std::all_of(m_files.cbegin(), m_files.cend()); constant iterator since the structure is unchanged.

Okay i understand, fair

Handle exceptions properly.

Add ability to get the zone definition name for error reporting.

Laupetin · 2023-12-23T16:13:17Z

src/Linker/Linker.cpp

-    "fastfile",
-    "ipak",
-};
+constexpr const char* PROJECT_TYPE_NAMES[static_cast<std::underlying_type_t<ProjectType>>(ProjectType::MAX)]{"none", "fastfile", "ipak", "iwd"};


Unfortunately clang-format does not behave particularly cool when it comes to array initializer lists.

You can force it to make way less awkward formatting by adding a trailing comma , after the last entry which is what i do everywhere where i catch it until they introduce a better way to handle this 😅

Laupetin · 2023-12-23T16:20:06Z

src/Linker/Linker.cpp

+        const auto iwdWriter = IWDWriter::Create(iwdFilePath.string(), &assetSearchPaths);
+        for (const auto& assetEntry : zoneDefinition.m_assets)
+        {
+            iwdWriter->AddFile(assetEntry.m_asset_name);


Does that mean it does try to write every asset into the IWD with the asset name interpreted as the full path?

That would mean that you cannot just simply use the same zone files as for the fastfile, right?
Because in a zone file for a fastfile you would write:

image,test_image

and it would refer to images/test_image.iwi.

In the context of the IWD here it would probably throw a "File not found"?

Hmm that is true. Would that mean IWDs should have their own type of definition that is only paths to files? Because you can plausibly load any kind of asset from raw in games such as CoD4/T4. I suppose it could be handled by making it so each asset type that doesn't have the full path in its name has a folder: for example image asset to images folder this way you wouldn't need to specify the images folder in the file name.

I mean while it's true that the older games support a lot of assets from IWDs, they should be consistent in what assets they put in which files, right? For example MW2 does only use IWDs for iwi images and streamed sounds.

I think some older titles also have weapon files in IWDs.

The way i'd do it is just have as much as possible in fastfiles and not in IWDs. At least for the first iteration.

Or what do you think?

Maybe I should add: The idea i had in mind on how to handle these other project types in the future was not to write different zone files necessarly.

I added a thing called "assetlists" for IPaks previously for example.
In the future i thought it might be neat to be able to refer to an assetlist of the written fastfile to just "put everything from the fastfile to an IWD or IPak".

Like Linker would link a fastfile mod.ff and create an assetlists/mod.csv file which can then be refered to from the IWD zone file with assetlist,mod.

That would require the way you define a zone and the way you define an IWD to match though 😅

Laupetin · 2023-12-23T16:26:13Z

src/Linker/Linker.cpp

+            return false;
+        }
+
+        std::cout << "Created iwd \"" << iwdFilePath.string() << "\"\n";


Is there a reason for mixing std::endl and \n? 😅

I know i did it but i try to stick to just \n atm since std::endl also flushes and that can cost a bit of performance.

Laupetin · 2023-12-23T16:27:17Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

@@ -0,0 +1,121 @@
+#include "IWDWriter.h"
+
+#include "../minizip/zip.h"


Please don't use relative paths in includes 😅 If minizip is currently not in the include paths for "ObjWriting" you may need to add it in the premake script "ObjWriting.lua"

Laupetin · 2023-12-23T16:28:25Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+          m_asset_search_path(assetSearchPath)
+    {
+        m_files.clear();
+        m_added_files.clear();


Clearing vectors in constructors is not necessary because the vector will be initialize by the automatically called default constructor of std::vector

Laupetin · 2023-12-23T16:30:09Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+        m_zipfile = zipOpen(m_filename.c_str(), APPEND_STATUS_CREATE);
+        if (!m_zipfile)
+        {
+            throw std::runtime_error("Error creating IWD file: \"" + m_filename + "\"");


Would be good to log this on std::cerr and have a graceful termination with non-zero exit code.
If you just throw an std::runtime_error this will unfortunately not be logged and will just crash.

And not being able to create a file may happen for example when having an invalid character in the path name.

Laupetin · 2023-12-23T16:31:44Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+        const auto existingEntry = std::find(m_added_files.cbegin(), m_added_files.cend(), fileName);
+        if (existingEntry != m_added_files.cend())
+        {
+            std::cerr << "Duplicate entry in " << m_filename << "'s zone definition; ignoring" << "\n";


you can just make it ...ignoring\n"; you don't need the extra output command 😛

Laupetin · 2023-12-23T16:32:50Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+        if (zipOpenNewFileInZip(m_zipfile, fileName.c_str(), &zi, NULL, 0, NULL, 0, NULL, Z_DEFLATED, Z_DEFAULT_COMPRESSION) != ZIP_OK)
+        {
+            zipClose(m_zipfile, NULL);
+            throw std::runtime_error("Could not open new file in IWD \"" + fileName + "\"\n");


Please don't use std::runtime_error 😅 This does not log anything and will just crash and not graceful exit

Laupetin · 2023-12-23T16:37:02Z

src/ObjWriting/ObjContainer/IWD/IWDWriter.cpp

+
+    ISearchPath* m_asset_search_path;
+    std::vector<std::string> m_files;
+    std::vector<std::string> m_added_files;


If you use an std::unsorted_set for this, this will not have to iterate through every file and do a basic string comparison for each previously added file on adding a file.

Sets make use of hashing and are therefore magnitudes faster for this.

JezuzLizard added 2 commits December 23, 2023 06:37

Add IWD project type.

c4a527e

Format using clang.

7fe01b2

diamante0018 suggested changes Dec 23, 2023

View reviewed changes

JezuzLizard added 2 commits December 23, 2023 07:47

Address review comments.

6d116d9

Handle exceptions properly.

Address review comments.

b8397b5

Add ability to get the zone definition name for error reporting.

Laupetin reviewed Dec 23, 2023

View reviewed changes

Laupetin linked an issue Dec 24, 2023 that may be closed by this pull request

IWD project type #27

Open

JezuzLizard closed this Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iwd project type #66

Iwd project type #66

JezuzLizard commented Dec 23, 2023

diamante0018 Dec 23, 2023

JezuzLizard Dec 23, 2023

diamante0018 Dec 23, 2023

JezuzLizard Dec 23, 2023

diamante0018 Dec 23, 2023

JezuzLizard Dec 23, 2023

Laupetin Dec 23, 2023

diamante0018 Dec 23, 2023

Laupetin Dec 23, 2023

diamante0018 Dec 23, 2023 •

edited

Loading

Laupetin Dec 23, 2023

diamante0018 Dec 23, 2023

Laupetin Dec 23, 2023

diamante0018 Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

JezuzLizard Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

Laupetin Dec 23, 2023

	constexpr const char* PROJECT_TYPE_NAMES[static_cast<unsigned>(ProjectType::MAX)]{"none", "fastfile", "ipak", "iwd"};
	constexpr const char* PROJECT_TYPE_NAMES[static_cast<std::underlying_type_t<ProjectType>>(ProjectType::MAX)]{"none", "fastfile", "ipak", "iwd"};

		@@ -0,0 +1,121 @@
		#include "IWDWriter.h"

		#include "../minizip/zip.h"

Iwd project type #66

Iwd project type #66

Conversation

JezuzLizard commented Dec 23, 2023

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

diamante0018 Dec 23, 2023 • edited Loading

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

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

diamante0018 Dec 23, 2023 •

edited

Loading