search

Secretchronicles / TSC

An open source two-dimensional platform game.
https://secretchronicles.org/
GNU General Public License v3.0
205 stars 49 forks source link

Changes to make msys2 build possible #642

Closed hgdagon closed 5 years ago

hgdagon commented 6 years ago

This is my very first PR. Please, let me know what I did wrong and I'll fix it.

TODO:

Quintus commented 6 years ago

Am 26. November 2018 um 00:46 Uhr -0800 schrieb hgdagon:

This is my very first PR. Please, let me know what I did wrong and I'll fix it.

This is very much appreciated. I can't review it right now, but I will do shortly in the next days. Overall, it looks good to me, but the devil often is in the details.

Thank you very much for your contribution!

Quintus

-- Blog: https://mg.guelker.eu PGP/GPG ID: F1D8799FBCC8BC4F

refi64 commented 6 years ago

@Quintus I meant me pointing it out was bikeshedding and useless, not that the command was useless, in an (evidentially failed) attempt to make sure it didn't sound serious... 😅

Quintus commented 6 years ago

I meant me pointing it out was bikeshedding and useless, not that the command was useless, in an (evidentially failed) attempt to make sure it didn't sound serious...

Mh, I'm sorry. Looks like it was me who has played the silly part here.

Quintus commented 5 years ago

Make sure this is still compatible with MXE

Ignore MXE for the moment. I'm inclined to say that if we have a working Win32 native build with MSYS2, we can drop MXE alltogether if it requires too much specialities in our build system setup.

hgdagon commented 5 years ago

I have a couple of questions.

  1. Does MXE build work (without these changes)? I tried building MXE, but ran out of space. I use my Linux laptop mainly for study, so I don't have a lot to delete at the moment. I'm mainly asking because of CEGUI DevIL Image Codec. CMake is configured to use the FreeImage codec for Windows, but I've tried it and I get undefined reference DevIL Image Codec. Msys2 doesn't provide static CEGUI libraries, and the DevIL Image Codec library requires 14 other DLLs (which is 14 more than anybody wants), FreeImage codec, on the other hand requires just one (FreeImage itself).

  2. How do you build the documentation? I installed the rubygems, but I can't find the target for documentation.

refi64 commented 5 years ago

How do you build the documentation?

https://github.com/Secretchronicles/TSC/blob/devel/tsc/Rakefile#L62

hgdagon commented 5 years ago

CPACK_PACKAGE_ICON - the name is confusing, it should be a BMP 150x57, ico is simply ignored, I tried BMP, but didn't show up at all (just white background) don't use Gimp. Also, this is what was breaking the NSIS build in msys2, needs to have the 4 slashes before the filename.

@kirbyfan64 I need to update Install.md then...

hgdagon commented 5 years ago

OK, now a big comment, since it looks like I'm almost done.

  1. As you can see I messed around with some of the documentation stuff, but I eventually removed the documentation generation mention in INSTALL.md. There are too many errors.

  2. I also removed the DevIL Image Codec from the TODO list. When I started this, I was adamant to use Devil and LibXML, since I wanted there to be virtually no difference between the Linux and Windows builds. And then I saw the DLL hell that it creates. The CEGUI package provided by msys2 comes with 3 image codecs: DevIL, FreeImage, and SDL2. The situation might change in the future, but, as of now, we have what we have. DevIL requires 14 unique dependencies, FreeImage - 18, while SDL2 - only 6. Using SDL2 would cut down the list of required DLLs from 52 to 45. So far, so good, except the SDL2 codec has issues.

  3. The final point in the TODO list proved to be a lot more difficult, than I thought it would be. SMC had something similar. Instead of a Message Box, I was thinking of a separate page in the uninstall wizard with 3 check boxes: 1 for savegames, one for screenshots, and one for config.xml. Other folders always deleted. With a final check to see if %appdata%\tsc is empty and deleting it if so. I know exactly how to do this in INNO, but the NSIS script is still a bit of an alien to me. I'm mentioning script, because it looks like the only way to this would potentially be CPACK_NSIS_EXTRA_UNINSTALL_COMMANDS, but it's pretty poorly documented.

  4. Finally, I'd like to mention some of the issues I've noticed in the game:

    TSC tries to load a directory as an image (Windows and Linux): Failed to load image "/usr/local/share/tsc/pixmaps". Reason: Image not of any known type, or corrupt - Linux Failed to load image "C:\TSC\share\tsc\pixmaps". Reason: Unable to open file - Windows

    Sliders, which (I assume) should be horizontal, are rendered as vertical (Windows and Linux). ScrollBars

    Window freezes during the "Caching Images" screen (Windows only). The window is "Not Responding" for the duration of caching and then the menu shows up.

refi64 commented 5 years ago

I always thought the slider was just a handle to drag up and down?

Window freezes during the "Caching Images" screen (Windows only). The window is "Not Responding" for the duration of caching and then the menu shows up.

This is Linux too. IIRC it's that something ends up holding up the main thread, and it ends up ignoring window events, making the system think it's frozen.

Would it be easier to just use Inno instead of NSIS? I'm familiar with it as well, and I don't think there was a particularly strong reason it was picked? (Of course @Quintus was the one who made the decision and would also know the reasoning behind it!)

Quintus commented 5 years ago

Sliders, which (I assume) should be horizontal, are rendered as vertical (Windows and Linux).

This is a known problem. Original SMC had a custom CEGUI skin with horizontal sliders. During the CEGUI 0.8 update, the custom skin was dropped in favour of the easier to maintain default CEGUI skin. The default CEGUI skin has no horizontal sliders, but only vertical ones. As the result, there are now vertical sliders with the dimensions of horizontal sliders. The correct fix for this is that someone changes the UI layout to use proper vertical sliding, but until now nobody has found the time. (please don't code that into this PR, it's outside its scope)

This is Linux too. IIRC it's that something ends up holding up the main thread, and it ends up ignoring window events, making the system think it's frozen.

Likely there's simply no event processing during image processing. The problem could (and should, probably) be quite easily remedied by extracting the caching process into a separate thread and only polling the thread's state in the main thread. (please don't code that into this PR, it's outside its scope)

Would it be easier to just use Inno instead of NSIS? I'm familiar with it as well, and I don't think there was a particularly strong reason it was picked? (Of course Quintus was the one who made the decision and would also know the reasoning behind it!)

There are two reasons why I've used NSIS:

  1. It is directly supported by CPack. No additional manual coding needed, the installer can be directly generated from CMakeLists.txt.
  2. It is supported by and included in MXE.

Reason 2 is irrelevant if we decide to drop MXE in favour of native building. Reason 1 depends on someone willing to code and maintain the installer manually.

I can't comment on the problem with the uninstall page.

I eventually removed the documentation generation mention in INSTALL.md. There are too many errors.

I will look after the problem after merging.

DevIL requires 14 unique dependencies, FreeImage - 18, while SDL2 - only 6.

TSC was originally an SDL1 application before we moved to SFML. It would be kind of funny to return to SDL just for the CEGUI rendering backend.... Your error messages however point to a CEGUI/SDL problem, not a TSC one. TSC itself will use whatever you provide it through CEGUI.

TSC3 drops CEGUI alltogether and just uses SFML for the graphics processing. Problem solved, dependencies cut off. Don't waste too much time in trying to make TSC 2's dependency tree sane.

@kirbyfan64 You should mark your reviews as "resolved" when you're done commenting on a section of code. At least that's what I suppose is what GitHub's review UI wants.

hgdagon commented 5 years ago

This is Linux too. IIRC it's that something ends up holding up the main thread, and it ends up ignoring window events, making the system think it's frozen.

On my Linux machine, I see the progress bar filling up as the images are cached...

Would it be easier to just use Inno instead of NSIS? I'm familiar with it as well, and I don't think there was a particularly strong reason it was picked? (Of course @Quintus was the one who made the decision and would also know the reasoning behind it!)

Like @Quintus said, no CPack INNO Generator. Although, this guy got me excited a couple of moths ago, but it looks like he's abandoned it :(

@Quintus That's it, then. If you need me to do any final touchups before merging, let me know.

Quintus commented 5 years ago

I'm fine with merging now. Now it's on @kirbyfan64 to mark his review as resolved, so we can merge :-)

refi64 commented 5 years ago

:+1: