Separate source code from compiled version

[plashenkov]

Currently the source code, compiled EXE, DLLs, all other files necessary for runtime, everything is in one folder. Maybe it would be nice to separate the source from binary files.

For example, two folders in root: source, bin. And LICENSE and README in the root as well.

[Falcury]

A /src or /source folder may be useful to reduce the clutter.
I do think it would be good to leave the binaries and Prince data files in the main folder. The original game is like this as well. And this way, less digging is needed if you want to just launch the game.

[NagyD]

Should I also move the Makefiles and the project files? I think I should.

[plashenkov]

Well, Makefile is not a part of a binary release. So I think yes too. :)
As I see it, the idea is to have a "clean" binary part, plus a clean (and comfortable for development) source part. To avoid a mess.
After that you can, for example, just zip the binary folder -- and you have a ready binary-only release.

[NagyD]

This is what I have now: https://github.com/NagyD/SDLPoP/compare/with-folders
I did not update CMakeLists.txt yet, because can't test it.
Is it enough to add /.. after ${SDLPoP_SOURCE_DIR}?

Also, should I move binaries to a bin/ folder (together with data/, *.DAT and SDLPoP.ini), or not?

[plashenkov]

Also, should I move binaries to a bin/ folder (together with data/, *.DAT and SDLPoP.ini), or not?

I think yes, but what do you think?

[Falcury]

Does this mean that we will stop distributing the sources with future ZIP releases?
I suppose in that case the doc/ folder would also have to be moved to bin/?

I think then we could use GitHub's release tags feature to mark historic code "snapshots" corresponding to each of the releases? Otherwise, it may become more difficult to get back to the exact source code as it was at the time of a particular release.

I agree that the approach has advantages. Though I also have no problem with continuing to distribute everything needed to build SDLPoP inside of the ZIP package. I do not have a real preference, I'll leave the decision to you.

I'll look into CMakeLists.txt (I assume adding /.. will work, but I haven't tried this yet).

[NagyD]

I do not have a real preference, I'll leave the decision to you.

I say we should keep the source in the ZIP releases.

[myself600]

My suggestion is to move *.{c,h} *Makefile CMakeLists.txt port_*.dev to src folder, and add ../ before prince{,.exe} where needed. Then you can use make -C src to compile the executable for GNU/Linux, etc.

Also, SDLPoP is buildable with CMake 2.8 so changing that in CMakeLists.txt might be a good idea too.

[Falcury]

I created a fast-forward commit for the with-folders branch: #71

[myself600]

Thanks, but I don't think this is enough. Instead of using a separate branch, this should be done directly in master (alternatively, this issue can be also fixed by creating a separate repository for SDLPoP data). Also, short names like bin, doc and src are more common and preferable (compared to full binary, documentation & source), while having a duplicate CMakeLists.txt in root folder kind of defeats the purpose of this proposal. Finally, removing the duplicate GUARD*.DAT and data/levels-original should be done too.

[NagyD]

Also, short names like bin, doc and src are more common and preferable (compared to full binary, documentation & source),

Currently the folders are data, doc and source.
So I guess the only renaming we need is source -> src?

Finally, removing the duplicate GUARD*.DAT

Those DAT files were added because, on Mac OS X, the PNG images are loaded as RGB instead of paletted: 4590ae0

[myself600]

So I guess the only renaming we need is source -> src?

It's just a suggestion. (yeah, only renaming only that one)

Those DAT files were added because, on Mac OS X, the PNG images are loaded as RGB instead of paletted: 4590ae0

Sorry, I forgot to check the commit description. So it's just another SDL-related issue then.

Edit: Misunderstood the question.

[NagyD]

Renamed: 1326f11
So, can I merge this into master now?
I want everyone to agree before I do something that big.

[Falcury]

having a duplicate CMakeLists.txt in root folder kind of defeats the purpose of this proposal

I remember having trouble with this; I think my IDE (CLion) expects the file to be present there, so that the root folder is registered as the "base project folder" (where CLion stores the project settings and such).
But maybe I missed something. I'm fine with deleting it if I can keep working in my IDE without issues. (I'll look into this)

Edit:
I found a solution for this: I should open the src folder as the base folder in CLion, then use the option Tools > CMake > Change Project Root to change the project root back to the parent directory.
https://www.jetbrains.com/help/clion/2016.1/changing-project-root-directory.html
After that, CLion will assume that the project name is "src", but this can be changed by editing the file src/.idea/.name.
Pull request: #79

So, can I merge this into master now?
I want everyone to agree before I do something that big.

Sure, for me it's OK!

[myself600]

Apologies for the delayed response.

@NagyD: As I mentioned above While it's not related to the issue, you should also delete levels-original and rename levels-test to LEVELS.DAT-test within the data folder (possibly in a separate commit). However, with-folders branch is currently lagging behind master and it's a mistery for me what will happen if you merge it without dealing with this situation first. Is Git going to merge only the extra commits, or will completely replace master with with-folders?

@Falcury: Thanks for resolving this. It'll make the repository a bit cleaner and easier to maintain.

As for GUARD*.DAT, maybe a separate bug report should be opened (even if this's Mac OS specific issue of SDL). Not a big deal though.

Edit: Realized that LEVELS.DAT has nothing to do with this issue.

[Falcury]

However, with-folders branch is currently lagging behind master and it's a mistery for me what will happen if you merge it without dealing with this situation first. Is Git going to merge only the extra commits, or will completely replace master with with-folders?

We could of course revert if it goes wrong... But I suppose the following should work in any case: first merging master into with-folders (similar to 0bd7b07), then with-folders into master?

[NagyD]

first merging master into with-folders (similar to 0bd7b07), then with-folders into master?

Done: b138aef
For some reason it shows only the "Merge branch 'master' into with-folders", but not the other way around.

Next thing is to update editor with the changes.
EDIT: Also done: f673532

And maybe editor could also be merged into master? I'm not sure about that.

[Falcury]

And maybe editor could also be merged into master? I'm not sure about that.

Some potential issues with the editor branch:

(although I say up front that the editor feature does work beautifully!)

• Right now, Alt+E can always enable the editor (by default this should not be possible!). If the editor gets bundled, I suppose it would be good to have an option enable_editor in SDLPoP.ini that is disabled by default.
• The editor feature changes the level format quite dramatically, because the number of rooms is made variable (through the compile-time macro NUMBER_OF_ROOMS).
  • Even with the default setting of 24 rooms, this causes compatibility problems, because the editor feature already adds 8 rooms for its own use (with the USE_EDITOR macro enabled). As a result, quicksave and replay files become unstable as well: SDLPoP consistently crashes if I quicksave, toggle USE_EDITOR on or off, recompile and try to quickload.
  • Backwards compatibility with Roomshaker, apoplexy, etc. is a concern (but less of an acute problem).
  • Need to recompile to change the number of available rooms, which is a bit painful.
  • I am not sure how the above problems could be fixed, particularly the headaches caused by the instability of the level format. I think the most workable solution for now would be to revert to max 24 rooms for now. If we want to support more than 24 rooms in the future we can always try to solve that problem separately (using some clever dynamic memory allocation, or whatever).
    Of course, reverting to 24 rooms is a problem for the palette mode implementation...
• I have a lot of trouble understanding the code in editor.c, it is quite abstract and could use some good documentation in my opinion. Or maybe it's just me :P

Another thought: perhaps it may be justified to offer the editor as a standalone download (at least for now)? Then the editor can have its own dedicated binary releases, so that the people who do not have the technical know-how to compile it themselves have the opportunity to play around with it, with the added freedom for the editor to support extra stuff (such as more rooms, more levels etc.) without worrying about compatibility too much. Then, if the compatibility issues become more manageable later on, we can always decide later to merge.

Also it may be a point of discussion whether people want/expect a full in-game editor to be part of the feature set. Personally I am open to the idea, but I can imagine that other people would prefer it to be a standalone add-on (maybe it can be as simple as an editor.exe and EDITOR.DAT that can be dropped into the SDLPoP folder, for instance?).

Paging @ecalot as well so he can weigh in.
(Apologies for going off-topic here.)

[ecalot]

Sorry about the editor documentation!

About the number of rooms:
I've decided to go for a static number of rooms (as a #define) to avoid dynamic memory complications and lots of debugging. If someone is planning to create a new mod, he should just decide the number of rooms at the beginning and distribute the corresponding .exe file with the mod, I don't think it's worth creating a prince.exe that dynamically supports all mods; specially because modders would like to edit their own .exe file to add some custom stuff and that's one of the major advantages of SDLPoP.

But there is something important on that: the game is compatible with Roomshaker, apoplexy, etc. as it loads the 8 rooms in dynamic memory but keeps the original file size untouched. The extra 8 palette rooms are saved in another file with other keycodes.

Anyways, I've got a branch in my fork called "top" with extended levels and rooms which is backwards incompatible.

About quicksave and replay:
Sorry about that, I've never used them nor tested with the editor. But those features are useful for players at playing time so disabling them in editing mode would not be a problem.

I've created the editor branch with the objective of building my own mod in mac because apoplexy didn't compile here (actually I've compiled it but it was unstable). Then I decided to go for a mod with lots of rooms so I've added the support. If I release the mod, I'll be releasing it with an executable file without the editor to avoid any problem. Advanced users who would like to edit levels there would have to use the github version.
As you can see, stability was the least of my concerns at the time, I just wanted the job done.

In my opinion, a separated executable is a great option. Although if you want to merge the versions go for it, I can help fixing quicksave and replay features or at least I can improve the source documentation.

[NagyD]

Can I close this issue now?

[Falcury]

Yes I think so!

So, then PR #48 can be closed as well (for now), I think?
I'll close #57 too (the scripting pull request); my progress on that has stalled a little and it is probably not yet good enough anyway.

[ecalot]

Done, PR #48 closed.

[NagyD]

Closed.