News:

Use the "Forum Search"
It may help you to find anything in the forum ;).

Terraformer refactor

Started by ceeac, January 23, 2022, 10:50:25 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

Found this one while cleaning up my repository. :)

This patch moves terraformer_t out of karte_t to separate files world/terraformer.h/cc. This alone reduces the size of simworld.h/cc by about 7.5%. I moved the code to a separate world/ directory because my intention is to eventually move world related classes there (e.g. simworld, simcity etc.) to reduce the number of files in the root directory. But moving terraformer_t to a different location would also be fine by me.

prissi

Personally I think we have rather too many folders. But of course this would depend on how much files would end up in world. (Also MSVC sorts files alphabetically, ignoring subfolder names, and transformer.cc would end far from word. So in MSVC using subfolder does not build any connection, only namint it World_transfomer.cc would carry an obvious connection ... )

On the actual implementation: Instead of making the non_check public, I think it would make sense to have them friend with transformer if possible.

Roboron

On the contrary, I think we have too many files in the source root. Although a big part of my problem with it is that many of those files are not related with code at all (like scripts) and also directories (like the simutrans data directory itself). For browsability I would rather see separate code and non-code files/folders than create more folders (but that is a reorganization so big and disruptive that I can't seriously propose it).

But back to the topic, I always find desirable to break code with thousands of lines into smaller chunks :_)

Dwachs

Fully agree with Roboron.

simworld.cc contains too much stuff (the game loop, the map, map creation, etc)

I would like to propose a reorganization of the directory structure. I.e., a src/ directory that contains just the source, an os/ directory that contains os-specific build scripts. There are now two directories script/ and scripts/, which is annoying.
Parsley, sage, rosemary, and maggikraut.

Andarix

Create a new branch and implement the restructuring.

If the result is approved, then it can be transferred to the main branch.

Humans are creatures of habit and find it difficult to accept change.

The second point is that such extensive conversions can lead to faulty versions. And I think that's one reason why prissi finds it difficult.

There is too much discussion at Simutrans and then it falls asleep.

prissi

It boils down to the different IDE. MSVC does not show any folders. The more files the less useful it becomes. Other IDE apparently differs.

So for MSVC starting files with gui_ or desc_ is most helpful. For other IDE (or just using an editor) it is folders. If we do the big folder moving and renaming exercise, then I propose also adding file prefixes obj_, veh_, ... , which would make things easier for MSVC users.

Also renaming/moving has to be done file by file on the svn (or git) command line (or using a graophical client), because with a patch the history is lost.

I would also say that there are more scripts (including a prepare environment script for various Linux like for msys2) which I think would be better in a "tools" folder.

However, I would like to release a 123.0.1 with the schedule fixes, to have a long term version before any big reorganisation. This requries again updating the pak128, pak128.german and pak48 isntaller, as well as getting a useful version for the Android build, so that I can finally upload it to the google store.

So please no shifting around until next weekend unless I release before that. Thanks.

Andarix

Quote from: prissi on January 24, 2022, 12:40:53 PM
...
However, I would like to release a 123.0.1 with the schedule fixes, to have a long term version before any big reorganisation. This requries again updating the pak128, pak128.german and pak48 isntaller, as well as getting a useful version for the Android build, so that I can finally upload it to the google store.
...

https://simutrans-germany.com/wiki/wiki/article256-unofficial-pak64-german-0-123-0-0-2

prissi

Ok, missed this as well ...

But since you have commit rights, it would be nice if you update
trunk/nsis/paksets.nsh
and
trunk/get_pak.sh

whenever you have a new version. That would automatically have the nightly with the newest paks, whenever the installer is run for the nightly.

ceeac

Quote from: prissi on January 24, 2022, 12:40:53 PMIt boils down to the different IDE. MSVC does not show any folders.
It does if the "Show all files" option is selected in the Solution Explorer.

In my opinion, moving the source files is a good opportunity to do some additional cleanup to the directory structure since most files are moved anyways. As long as the new file/directory structure is not finalized, keeping track of the changes using a script is better than using a branch, I think. This will also help to eventually port the changes to Extended. I have attached a quick-and-dirty quality script to provide a basis for discussion. Some comments in no particular order:

  • I think it is preferable to have simutrans/makeobj/nettool and the squirrel sorces all separate in a subdirectory of src/; especially the squirrel sources should be separate because they are from an external source.
  • Since ways and vehicles are derived from obj_t, these should be inside obj/.
  • loadsave_t is one of the main classes for doing I/O, so I moved it into io/
  • There is now a separate directory for tool related files. This should help with breaking up simtool.h/cc into smaller chunks in the future
  • Scripts are now all in scripts/. I do not have a strong opinion on whether the directory is named scripts or os or tools
  • There is a new directory world/ for {simworld,simcity,simplan}.{h,cc}. Maybe simfab should be moved there, too?
Additionally, maybe files should be renamed to match the name of the class they contain (e.g. simplay.{h,cc} -> player.{h,cc})?

Quote from: Dwachs on January 24, 2022, 07:07:49 AMsimworld.cc contains too much stuff (the game loop, the map, map creation, etc)
I agree. I think moving just the basic map properties (size, planquadrats, grid, lookup() and friends etc.) to a separate class and deriving karte_t from that would help a lot.

Now finally back on topic. ;)
Quote from: prissi on January 23, 2022, 12:30:49 PMOn the actual implementation: Instead of making the non_check public, I think it would make sense to have them friend with transformer if possible.
I disagree. In my opinion the *_nocheck methods should be public because they are just variants of existing (public) methods, and the possiblity to accidentally mix the two variants is low because of the _nocheck suffix. This will also work well with the suggestion in the previous paragraph, whereas the friend declaration would not.

prissi

This script will lose the history. I tried mv with pak128 and it did not work. Will need svn mv, which needs to have the target under source control; I added a script that actually works.

Also one will lose the ability to use branches with gitsvn, but since nobody uses branches on the SVN this is probably not an issue.

I just put also the OS dependent code under src, but I am open to discussion.

Dwachs

Looks good to me. Maybe one can move OSX/ and android into sys/ ?
Parsley, sage, rosemary, and maggikraut.

prissi

My motivation for not putting into sys was that sys contains the backend C sources and OSX, android, and Windows contains mostly non-C soucre files (Manifest, scripts, patches, installer). SO I though not putting them next to the C-code.

What about renaming some files to start with prefixes (obj_ vehicle_  gr_) and renaming the whole german boden and wege files ... , since we have to touch anyway almost all includes.

ceeac

Quote from: prissi on January 25, 2022, 12:02:43 PMWhat about renaming some files to start with prefixes (obj_ vehicle_  gr_)
As long as the class the file contains is renamed to match the new filename, I am okay with this, although personally I prefer the current naming. Otherwise there are too many special cases where the class name does not match the file name and that increases cognitive load especially for people unfamiliar with the code.

Quote from: prissi on January 25, 2022, 12:02:43 PMand renaming the whole german boden and wege files
There are also some files in obj/ like baum, gebaeude and leitung2. These should be renamed, too IMO.

There are also files that have a _t suffix; I have added a line to the script to remove the suffixes.

Roboron

Quote from: prissi on January 25, 2022, 12:02:43 PMrenaming the whole german boden and wege files

Maybe the bauer files too, I still have nightmares with those.

The *.ico files are not code and windows only, so they are better suited for the windows folder. Well, simutrans.svg is also not code, but there isn't an appropriate place for it. Maybe os/{windows/linux/macos} folders in the root directory where non-code os-specific files could go (mentioned *.ico files for windows, .svg and .desktop for linux, .icns for macos, etc...). The OS stuff is definitely a bit tricky.

Talking about macos, it may be a good idea to rename "OSX" to "macos" (as it was rebranded years ago) since we need to change the paths anyway.

Quote from: ceeac on January 24, 2022, 07:17:28 PMI think it is preferable to have simutrans/makeobj/nettool and the squirrel sorces all separate in a subdirectory of src/; especially the squirrel sources should be separate because they are from an external source.

Pretty good idea.

It's a breeze not having to scroll down through dozens of files to reach the project build files. They are much more accessible now :_)

prissi

The while statements does not work on msys2 ...

Quite some files are not named like their classes, or the classes they belong to. Some files are even classless. Anyway, here is a version that also tries to repair the includes and build files. MSVC almost builds it, but I have to stop right now, will continue later maybe.

And where can I see the folders in the source explorer or the window tabs in MSVC? The option seems very well hidden. Stack overflow even says Microsoft never bothered to add this?

Dwachs

Some time ago I wrote a script, translate_code/batch_replace.sh, to take a csv file and use it for automatic replacement. I used that to translate things.
The csv files were tuned such that after one run of the script the program compiles.
The idea was that one can apply the same script to a patched version. Maybe we can use this here as well.
Parsley, sage, rosemary, and maggikraut.

prissi

I had these here, but then I would need to built a csv file and the temparary file was not kind to svn mv ... Also with some path changes the include handling was more complex. Even more since MSVC needs \ or \\  and does not accept / inside several files. This script will be run one time, and cMake* files will need manual chnages, since the various backend and clipboard etc. files are deep in lines.

The Makefile will likely also need a few changes, and the android ... let see after the next release.

prissi

OK, here is the final state. I give up on cmake. It seems it wants to do fluidsynth on Mingw even though this should be optional, since there is the Win32 backend.

Simutrans Makefile and MSVC project files work fine. (Makeobj and nettool not tested.)

Roboron

Well you wrote:

sed -i 's| sound/| src/simutrans/music/|' cMakeLists.txt
sed -i 's| music/| src/simutrans/sound/|' cMakeLists.txt


So no wonder why it wasn't working as intended XD

I have fixed it for you, and continued working on CMake until it was completely usable for MinGW and Linux. MacOS is still not there, nor is nettool or makeobj, but I don't have time for more for now.

PJMack

If the simworld.cc is to be split up and the files moved, it may also be worth considering to split up simtool.cc as it is also getting a little long with multiple classes.

ceeac

I am on the fence about this - On the one hand, this decreases the amount of code per file which is good, but on the other hand compile times will increase significantly if every class gets its own file.
Maybe it is a good idea to move tool base classes (kartenboden_tool, two_click_tool etc.) to separate files and group general/simple/dialog tools into a file each?
Speaking of tools, there are still simtool*.h/cc files in src/simutrans/, imo they should be moved into src/simutrans/tool/.

prissi

Well, those I took form the original script. But of course moving simtools at this time as well. I think one coukd split simtools in two parts for general, and simple and dialog tools. There are probably not enough cc code for dialog tools; but one may even split it in three.

ceeac

New version - fixes compilation of makeobj/nettool for Makefile and CMake, and moves simtool files into src/simutrans/tool/.

prissi

#23
Thank you.

That rather works by accident, since none of your dots and $ are escaped in sed. Or does unix use another kind of sed.

Anyway, it seems to compile. I think the rest can be done by hand. If nobody opposes this, I will run it tonight and then fix the github workflow by hand.

ceeac

I think it is best to apply the original patch first and then run the script. Other than that, no objections from me. :)

Dwachs

Quote from: prissi on February 03, 2022, 05:23:29 AMIf nobody opposes this, I will run it tonight and then fix the github workflow by hand.
ys, please move ahead
Parsley, sage, rosemary, and maggikraut.

prissi

comitted in r10444. I did not had time to look at the github workflow, but I would no be surprised if it is broken by this. Will probably fix it tomoorw, after it made it way on github.

prissi

Ok, the nightly builds in github seem to work as well. I think this project came somewhat to an end. Further reorganisation should rather open a new thread.

Dwachs

distribute.sh is still broken as it assumes the old directory structure. I did not touch it, as I never worked with this file. Otherwise the stuff at github works
Parsley, sage, rosemary, and maggikraut.

prissi

It worked for me on my github fork. Maybe I forgot to patch it back.

Dwachs

Parsley, sage, rosemary, and maggikraut.