Improve readability of main.cpp.

[ronyeh]

1. Be consistent with spacing in if, else if, else blocks.
   Removed empty lines at the beginning and end.
   Previously, some blocks had no extra empty lines, while other blocks
   had one at the beginning, end, or both.

2. Add underscores to variable names like hasicon and iconpath.
   Other files used icon_path, so now main.cpp does the same.

3. Reduce the number of times OS::get_singleton() appears.
   Some functions like print_help() look less visually noisy. This helps
   the reader while skimming the source code.

   Added OS *os = OS::get_singleton(); to the eight functions that use the
   OS singleton. These functions were modified:

print_help(const char *)
test_setup()
test_cleanup()
setup(const char *, int, char * [], bool)
setup2(Thread::ID)
start()
iteration()
cleanup(bool)

[ronyeh]

Added an underscore to some variables to be more consistent with the rest of the code base, which uses _ to separate words.

[AThousandShips]

Nowhere else do we grab the singleton like this as far as I know (for other ones there are cases, like EditorUndoRedoManager), I don't think it's the recommended procedure, and not convinced it changes readability, in fact it makes it less readable to me as I now at first glance assume os is not OS::get_singleton() as it breaks with established practice

[YuriSizov]

Thanks for opening a PR. This file is slated for a rewrite, because it is indeed very messy. I'm not convinced these changes help in the meantime. Some are arbitrary (like splitting a get singleton oneliner into two lines), and some make things less readable for such huge spaghetti methods (like removing a bunch of empty lines).

[kleonc]

Nowhere else do we grab the singleton like this as far as I know (for other ones there are cases, like EditorUndoRedoManager)

E.g. in TileMap.cpp:

// 10 occurences:
RenderingServer *rs = RenderingServer::get_singleton();

// 9 occurences:
RenderingServer::get_singleton()->...

// 8 occurences:
RS::get_singleton()->...

godot/servers/rendering_server.h

Lines 1693 to 1694 in 21d080e

	// Alias to make it easier to use.
	#define RS RenderingServer

I mean it seems it's not well settled what's preferred (even within the same author). 🙃

Personally I don't mind OS::get_singleton()->... to os->... changes but at the same time I'm not convinced it's needed. If going for such changes then maybe it would make more sense to settle on one specific style, and change accordingly all relevant ...::get_singleton() occurences accross the whole codebase at once (and stick to such choosen style in the future).

[AThousandShips]

My bad I didn't know the situation in TileMap it looks like an anomaly though from where I've looked

[ronyeh]

@YuriSizov No problem. I view PRs as a way to start a conversation. I am happy to take on the rewrite of main.cpp if someone in power authorizes me to. Is that person you? :-)

I am new to Godot engine, so I am reading the source code. I noticed that main.cpp was a bit difficult for me to skim. My reasoning for reducing the number of times OS::get_singleton() is called is that I believe it reduces the visual noise and aids the reader of the source file.

The singleton getter appears 243 times in this file, and replacing it with os reduces the line width by 17 characters. This helps people who code on laptops, since some of the lines are over 200 characters long.

We are not really consistent with whether we save the singleton to a local variable. For example, in find_in_files.cpp we have this:

	OS &os = *OS::get_singleton();
	uint64_t time_before = os.get_ticks_msec();

Anyways, we can continue the conversation in this thread, but I can slowly improve main.cpp, piece by piece. Is that OK with you all?

The first PR I could submit would be to rename some of the variables. I find hasicon confusing at first glance.

Later on, we can extract some functions and reorganize the file.

This would require lots of testing, since main.cpp has lots of branches, flags, and ifdef blocks. Luckily, it is main.cpp so any bugs I introduce by the refactor would probably be discovered very quickly! :-D

[YuriSizov]

if someone in power authorizes me to. Is that person you?

That's not how we approach things :) You can work on it, eventually, but it needs to be discussed first. main.cpp is too important to start breaking it without a clear plan for the entire process in place. I'm not against smaller codestyle improvements in the meantime, but they also need to be warranted.

Here, I can agree that the print command can be made a bit clearer. But then you also do this in a few places where the singleton is only accessed once. We don't normally split such lines, and if there is no consistent style, as kleonc points out, these changes are pretty arbitrary and are not really warranted by anything but personal taste. If it was done as a part of the general reorganization of the file, then great, that's not a big deal. But as a standalone PR it makes it low value.

As for the refactoring itself, it needs a comprehensive proposal. Something that aggregates all the entrypoints, use cases, permutations that the file presents. There is quite a bit, so I'm not sure if you'd be well equipped to make such a proposal if you're only starting with the engine codebase. But of course, try by all means, we can polish it in the process. Don't want to discourage you, just want to point out that this is a pretty big undertaking.

[Calinou]

See also #49362 and #44594.

[ronyeh]

Don't want to discourage you, just want to point out that this is a pretty big undertaking.

No problem. I am not easily discouraged.

Thanks @Calinou for linking the other PRs.

As we can see, many developers start with main.cpp and notice that it has some readability issues. This means that their first impression of Godot engine is not ideal.

I feel like a huge refactor of main.cpp is risky and also is unlikely to be merged due to many stakeholders chiming in with competing opinions.

Here is my suggestion:

• PR with just the rename of some variables to add underscores, like hasicon -> has_icon
• PR with the extraction of OS::get_singleton()-> to os in just the print_help(const char *) function. That was my original intention, and I agree that some of the other ones aren't necessarily useful. It is just that I wanted to be consistent throughout the entire file.

Would starting with those two PRs help? I think it is much more likely that main.cpp improves slowly, like replacing pieces of the Ship of Theseus. A full refactor in one PR would probably not be merged without intense discussion and agreement across all the main stakeholders.