diasurgical / devilutionX

Diablo build for modern operating systems
Other
7.99k stars 781 forks source link

Failed to open file: fonts\22-00.bin File not found #3043

Closed efedo closed 2 years ago

efedo commented 2 years ago

Important information Windows 10 / c39c6ce / Compiled myself (MSVC 16.11.3)

Describe Fatal error appears immediately after Hellfire splash screen.

Steps To Reproduce

  1. Clone fresh repository, compile and run. image

Additional context Regression post-66e1e38. Appears to occur because devilutionX.mpq is not built by default but is now required for font handling.

StephenCWills commented 2 years ago

You'll need to install smpq to build devilutionx.mpq. building.md has links and instructions.

efedo commented 2 years ago

Now that devilutionx.mpq is required smpq should be installed by default. It's a bad look to not have the CMake configuration work out of the box.

StephenCWills commented 2 years ago

It works fine if you follow the instructions. It's a prerequisite like vcpkg. I don't understand the distinction.

Well, if you can get it to work, feel free to submit a PR.

efedo commented 2 years ago

It's nothing at all like vcpkg. Vcpkg is widely used and installed by default in most dev environments. New users trying to compile devilutionx for the first time are going to either already have vcpkg or be able to set it up without difficulty based on any number of installation guides.

Smpq is by comparison an obscure library. As far as I can tell, it doesn't have any binaries or packages available for Windows. And building.md has no instructions for how to compile or install it on Windows or most other platforms. This is going to deter most newcomers on non-*NIX platforms from compiling devilutionx.

But by all means, continue with your rude attitude.

StephenCWills commented 2 years ago

I explained to you what you needed to build smpq, and explained that it's already in the build instructions to justify closing the issue. I also explained that it is a prerequisite, like vcpkg, and that I didn't understand what you were getting at, but suggested that you could submit a PR since it seemed you felt strongly about it. Considering this issue exists in the first place because you didn't follow the documented build instructions, I feel I've been more than gracious. I ask that you tone it down.

efedo commented 2 years ago

The build instructions mention smpq is an optional dependency on a limited subset of platforms, but it appears to now be a required dependency on all platforms. Smpq, and packaging of devilutionX.mpq, is likewise treated as an optional dependency in the CMake configuration, even though it is now required.

AJenbo commented 2 years ago

It's not a requirement, you can get devilutionx.mpq by other means.

efedo commented 2 years ago

Isn't devilutionx.mpq now required for devilutionx font handling, making it a requirement?

AJenbo commented 2 years ago

It's a runtime requirement yes, but not a build time requirement. Hence it's not required for the build process to succeed.

StephenCWills commented 2 years ago

FYI, it should also work without devilutionx.mpq if you just place the resources in your data path alongside diabdat.mpq.

efedo commented 2 years ago

Having build instructions and a default CMake configuration that no longer produce a functional application without manual (and undocumented) copying of dependencies puts up a considerable wall for newcomers who are compiling for the first time and may be interested in joining your community. It may also limit the platforms on which devilutionX (and all dependencies) can be built, if smpq is not readily available for all build platforms.

It just seems rather like a shame, since this is a considerable regression in the user-friendliness of the build process compared to just a few weeks back. Personally I would strongly suggest avoiding the issue by switching to a newer package format for original content that does not require an obscure binary dependency. MPQ was great for its day, but there are better options going forward.

AJenbo commented 2 years ago

Having build instructions and a default CMake configuration that no longer produce a functional application without manual (and undocumented) copying of dependencies

You will always have to provide the original game data your self.

It just seems rather like a shame, since this is a considerable regression in the user-friendliness of the build process compared to just a few weeks back.

It's a trade of like many things, having the MPQ be generated makes things a lot easier for the artists, making them less dependent on the programmers, giving them more time to write meaningful code.

Personally I would strongly suggest avoiding the issue by switching to a newer package format for original content that does not require an obscure binary dependency. MPQ was great for its day, but there are better options going forward.

Removing MPQ support would require users to have to convert the original archive before being able to run the game. We do have plans to add support for zip files, but have not had time to do so yet. If this is something you would be interested in working on it would be greatly appreciated.

StephenCWills commented 2 years ago

Having build instructions and a default CMake configuration that no longer produce a functional application without manual (and undocumented) copying of dependencies puts up a considerable wall for newcomers who are compiling for the first time and may be interested in joining your community.

Perhaps the simplest solution to this would be updating the documentation, which would also be a noble pursuit if you'd like to work on that.

NiteKat commented 2 years ago

Removing MPQ support would require users to have to convert the original archive before being able to run the game.

Wouldn't it be possible to still support MPQ for the original archive, while supporting some other format for new content? I don't feel like @efedo was suggesting dropping MPQ support altogether, just for new content that is added such as the fonts.

qndel commented 2 years ago

Removing MPQ support would require users to have to convert the original archive before being able to run the game.

Wouldn't it be possible to still support MPQ for the original archive, while supporting some other format for new content? I don't feel like @efedo was suggesting dropping MPQ support altogether, just for new content that is added such as the fonts.

We do have plans to add support for zip files, but have not had time to do so yet

🙄

NiteKat commented 2 years ago

Removing MPQ support would require users to have to convert the original archive before being able to run the game.

Wouldn't it be possible to still support MPQ for the original archive, while supporting some other format for new content? I don't feel like @efedo was suggesting dropping MPQ support altogether, just for new content that is added such as the fonts.

We do have plans to add support for zip files, but have not had time to do so yet

🙄

Well, @AJenbo 's post was suddenly talking about full removal of MPQ support which seemed out of place. 🙄 lol

Editing to add: I think Ajenbo's comment comes from the word "original" in the post to which he was responding, which I interpreted as "New content made by someone so it is "Original"" as opposed to "Originally shipped with the game." So I see why Ajenbo made the comment he did. :)

efedo commented 2 years ago

Yes, that is correct @NiteKat. If everyone is okay with that, I'll dig around and see how much effort would be required to add zip support.

Alternatively, we can at least repost the abandoned smpq source (GPL) on github, so that smpq can be automatically installed in the CMake config. I'm assuming that people don't want to bundle smpq as a third-party lib due to license issues?

Edit: Oh! I didn't even think about the different interpretations of original. English is such a funny language. Yes, I meant original as in "new", sorry.

StephenCWills commented 2 years ago

Alternatively, we can at least repost the abandoned smpq source (GPL) on github, so that smpq can be automatically installed in the CMake config.

FYI, that source is being used to build smpq for the Vita CI build. https://github.com/diasurgical/devilutionX/blob/master/.circleci/build-and-install-smpq.sh

It looks like there was more to it than just "plug it into CMake and profit". This script got CI working again, but it doesn't seem ideal for integrating with CMake.

I'm assuming that people don't want to bundle smpq as a third-party lib due to license issues?

To be clear, smpq is a tool, not a lib. It allows us to run smpq -M 1 -C PKWARE -c "../devilutionx.mpq" ... on the user's system to create the MPQ file from the individual files in Packaging/resources/assets. As was previously discussed, this is an optional step in the build process, and so it makes some sense to have the user opt in by installing the necessary tool.

More to the point, however, we worked pretty hard to avoid bundling prebuilt binaries and even source code for a significant portion of our third-party dependencies. We did that for at least a couple different reasons that had nothing to do with licensing, but licensing could be a factor as well. Regardless, we don't want to backpedal and start dumping third-party code into our repo again. At the very least, especially since there exists a documented process for installing smpq on user systems, it absolutely does not warrant that kind of treatment.

Although you are not the first Windows user in recent history that has expressed an aversion to following instructions for manually setting up their build environment, I do not personally have any interest in going out of my way to accommodate your whims. If you can demonstrate that you have actually tried and struggled to follow the build instructions, I would appreciate constructive feedback. If you would instead like to try to smooth out the build process using CMake FetchContent, feel free to do so yourself and submit a PR. We have plenty of examples to work from.

Of course, I say all this knowing that you intend to work on zip support for now. If that works out, you can disregard these suggestions as they should no longer be relevant.

efedo commented 2 years ago

Although you are not the first Windows user in recent history that has expressed an aversion to following instructions for manually setting up their build environment, I do not personally have any interest in going out of my way to accommodate your whims.

Is this kind of platform hostility/snobbery really necessary nowadays? We're not on 90s Slashdot. This issue also affects most platforms, not just Windows. I am concerned about how difficult this makes it for newcomers to successfully compile and run the package.

Put yourself in the shoes of the average github user stumbling across this project for the first time. They clone it, follow the build instructions fully and... "Failed to open file: fonts\22-00.bin File not found". If you're trying to deter new people from joining your project that's a great way to do it.

Anyway, I'll add zip support to make the code side of this a non-issue.

StephenCWills commented 2 years ago

Is this kind of platform hostility/snobbery really necessary nowadays? We're not on 90s Slashdot. This issue also affects most platforms, not just Windows. I am concerned about how difficult this makes it for newcomers to successfully compile and run the package.

Rather than automatically assuming everything I do or say is rude or snobbish, perhaps you should start by learning a bit about this project. I singled out Windows users because that's where we've been getting all our complaints. It's as simple as that.

Put yourself in the shoes of the average github user stumbling across this project for the first time. They clone it, follow the build instructions fully and... "Failed to open file: fonts\22-00.bin File not found". If you're trying to deter new people from joining your project that's a great way to do it.

I know for a fact that if a user receives that error, they did not follow the build instructions fully. They are missing devilutionx.mpq, and the instructions explain how to build devilutionx.mpq. I don't know why you continue to insist that the build instructions don't include this step because you have repeatedly been told that they do. I'm not sure what you stand to gain by pushing so hard on this narrative, but it's verifiably false by anyone with eyes and an internet connection so I wish you'd stop.

efedo commented 2 years ago

I know for a fact that if a user receives that error, they did not follow the build instructions fully.

That is unfortunately incorrect, which, as you so politely put it, "anyone with eyes and an internet connection" can verify. The current build instructions discuss installation of smpq on only a limited subset of platforms, and even then only as an optional component of the build system, with wording such as "If you want to build the devilutionX.mpq File (optional)".

Alas, we are going in circles and this will be a non-issue shortly.

StephenCWills commented 2 years ago

That is unfortunately incorrect, which, as you so politely put it, "anyone with eyes and an internet connection" can verify. The current build instructions discuss installation of smpq on only a limited subset of platforms, and even then only as an optional component of the build system, with wording such as "If you want to build the devilutionX.mpq File (optional)".

The absolutely mystifying thing is that you started this issue knowing full well that you were missing devilutionx.mpq and yet you still insist the build instructions are lacking. You reference the Linux instructions even though you indicated you're building on Windows 10 using MSVC. Even supposing we had a new user on Linux who skipped that step, they can always go back and follow the instructions again to make sure they didn't miss anything, or they can search through issues and find my response to this one. If all else fails, they can open an issue here and I will be happy to close it and give them the information they need. Within the next few weeks, we'll have v1.3.0 which will be shipped with the latest version of devilutionx.mpq so it will not even be necessary for users to locate the latest test builds or ask Qndel to find a prebuilt copy.

There's a lot going on here that you don't know about because you just walked in our front door and started ranting about everything after hitting a little stumbling block. And this is my opinion, but I think that if you were truly the champion of the newbie you claim to be, you would just fix the documentation and submit a PR citing the font error as a potential source of confusion for newcomers. Going after the zip task first suggests to me that you don't actually care about newcomers and would rather find a more interesting task to occupy your time. I'm not saying there's anything wrong with that, but I can't take your grandstanding seriously.

Alas, we are going in circles and this will be a non-issue shortly.

This was always a non-issue, hence why I closed it.

AJenbo commented 2 years ago

@efedo I would expect people who want to get the application up and running to also, preferably first, take a look at the installation instructions where devilutionx.mpq is clearly listed as required for running the application, and ways to obtain it are mentioned.

Also please keep in mind that the project and code is actively changing and large changes may require a few iterations before everything is smoothed out.

I know for a fact that if a user receives that error, they did not follow the build instructions fully. They are missing devilutionx.mpq, and the instructions explain how to build devilutionx.mpq. I don't know why you continue to insist that the build instructions don't include this step because you have repeatedly been told that they do. I'm not sure what you stand to gain by pushing so hard on this narrative, but it's verifiably false by anyone with eyes and an internet connection so I wish you'd stop.

I think at this point @efedo is acknowledges that it's there, but is saying that it being optional can lead the mentioned error, if you also skipped over the alternatives.

I think it would be good if we all reset our views before continuing this debate.

Maybe it would be enough to stipulate that if they skip it, they will need to download devilutionx.mpq instead. Copying the raw assets automatically in case smpq isn't found unfortunately doesn't appear to be an option as we can't reliably determine the data path at build time.

StephenCWills commented 2 years ago

@efedo I would expect people who want to get the application up and running to also, preferably first, take a look at the installation instructions where devilutionx.mpq is clearly listed as required for running the application, and ways to obtain it are mentioned.

There actually is an issue there, because it's still referencing the file from Packaging/resources which no longer exists. We could point to the 1.2.1 version of the MPQ, but that wouldn't work for the latest build. Providing a link to a single version of the mpq just invites similar errors to the one @efedo encountered as we implement new features that require additional assets. And adding support for zip won't even fix this until v1.3.0 is out anyway so we're back to square one.

I personally think there are more pressing concerns, and documentation will have to be updated when v1.3.0 is released so I vote we simply point to the 1.2.1 version of the MPQ and leave the build instructions as-is until we get to that point. Having said that, we're in the midst of Hacktoberfest so maybe some would feel that changes priorities a bit in regards to the build instructions.

I think at this point @efedo is acknowledges that it's there, but is saying that it being optional can lead the mentioned error, if you also skipped over the alternatives.

That's why I'm saying he should update the documentation. It's not explicitly stated as optional in the Windows build instructions, but rather that you'll need it in order to build devilutionx.mpq. From there, I feel the user can determine whether they'll need it or not. That should have been enough for @efedo when he posted this issue so I feel his concerns have been addressed. Maybe Linux users will stumble in the same way, but I've yet to see an issue raised so I'm not convinced that this is a major issue.

efedo commented 2 years ago

Thanks @AJenbo. Do you have any objection if I pack new content in a zip archive rather than an MPQ? It just seems like the most straightforward way to eliminate this issue.

@StephenCWills: I have not started "ranting about everything", only about the rude and dismissive attitude of a single individual. Thus far everyone else on here has contributed productively to the discussion. Game dev/open source culture has evolved over the past few decades, and there is now a certain level of expected decorum, which you fight at your own peril. But, since I have, as you put it, barged through your front door, I will say no more on the matter, and let your community handle it however it chooses.

AJenbo commented 2 years ago

This conversation has gotten out of hand with insults, accusations, and misunderstandings flying in multiple direction. I'm locking the conversation. Please move on to other subjects.

Thanks @AJenbo. Do you have any objection if I pack new content in a zip archive rather than an MPQ? It just seems like the most straightforward way to eliminate this issue.

It's a planned feature, if you work on it we will appreciate the help.

julealgon commented 2 years ago

The absolutely mystifying thing is that you started this issue knowing full well that you were missing devilutionx.mpq and yet you still insist the build instructions are lacking.

I think he was coming at this from the perspective of a newcomer/a person with less experience, so the comment made sense in that context. Even though he would be able to make it work, some people would not and would become stuck using current instructions.

Question: why don't we just copy the assets to the target build folder automatically? That doesn't require the MPQ generation tool, and would work just fine since we allow loading assets directly.

I have to say that I tend to agree 100% with the concern being raised here, as a huge proponent of making the project "just work" with minimal manual intervention myself.

I think its fine to have MPQ generation be completely optional, but the standard build should still work fine without it.

AJenbo commented 2 years ago

@julealgon the issue is locked, please do not post in it, use the chat instead.