Refactor and document source code, get rid of technical debt

[bliepp]

Feature/enhancement description: First of all: Great software. I really appreciate the features so far and I like the pace of development. I also would like to contribute but as I glanced over the source code I found it very difficult to get started. I think the code is not very self explanatory and it could use some docstrings (and further documentation in general). I cannot see any core concept in terms of software design. Also most classes heavily mix responsibilities they should not cover and seem to be highly entangled. I regularly stumble upon functions I have no clue what they are doing so I have to trace them back in some heavily nested and entangled function calls. The confusing naming many times does not really help.

To give a specific example: Looking into the Preview 3D panel folder, most scenes only contain few nodes and it is not immeadiately clear which scene imports which. For example preview_3d_panel.tscn inherits preview_3d.tscn, but is the only inherited scene. The actual 3d scene is called preview_3d_scene.tscn and has a child scene dedicated for the objects displayed called preview objects.tscn. All scenes in this case only have a hand full of objects and some of them could possible be merged for better overview. The root node of an instanced child scene often has a different name than in the scene itself (e.g. the 3d preview scene's root node is called Preview3DScene in the scene itself, but Preview3D when it is instanced, while there exists a scene called preview_3d.tscn. That's confusing as hell.). As you might see the overall structure is very unnecessarily complex and questionable in terms of software design.

The code to the root node of preview_3d.tscn not only has multiple responsiblilities it should not have (e.g. it handles map generation but is a ViewportContainer while it is also responsible for 3D navigation and creating the menu dynamically on the fly by calling a magic parent object making assumptions for it), but also has mutliple different methods for creating maps without a obvious distinction between them (e.g. generate_mesh_normal_map vs. do_generate_mesh_normal_map). Sometimes child objects are loaded into member variables using onready, sometimes they are called multiple times via the dollar operator. Also, on many occasions scenes are (pre)loaded inline inside of functions. Many of these issues could be avoided by splitting it up by different responsibilites, maybe utilizing something like design patterns, and adding docstirngs for unclear code. Also, relying more on class_names making classes would definitely help in many occasions increasing readability.

Conclusions Overall I discribed just one specific feature and most others are equally confusing. I think Material Maker could really use some heavy refactoring before it is to late. With a piece of software at this pace of development and a feature growth like this non-refactored and non-documented code increases technical debt exponentially making it harder and harder to contribute. When looking back to older code bases it becomes clear how code quality decreased over time and I think it's a pity. At some point in the future it might be too costly to implement a nice feature because of this progression, even though it might be something easy with well-written code. I cannot understate how important a clean code base is.

I really hope you don't take this as an insult as it is not meant to be one by any means. It is not even a fault based on bad development, it is rather a natural development of large code bases. I would happily help out as good as I can but this seems to be something ground breaking, so it is not something that can be done with a simple single PR.

Best bliepp

[bliepp]

Oh, I forgot to mention in my example that the code for 3D navigation is written a 50 line long function. This too long and very non-modular.

I had the idea to implement alternative navigation styles to the 3D viewport and wanted to try it out before creating an issue and a PR, but even though this is a simple feature there was no chance I could have implemented it in clean, modular and future-proof way. A dedicated class to handle navigation to inherit from would have made it easy, but the way it is implemented does not give one a chance to modify anything.

I also wanted to add additional material exporters for different game engines/3D software, but until now I could not find an entry point for that.

[RodZill4]

Yeah you're completely right, there are parts of MM that need refactoring. But migration to Godot 4.0 will require a near full rewrite, so it would be a waste of time to do this now. Plus ports to additional platforms (Android...) will also require code/scenes to be reorganized. I don't want to rush this, because I'm not sure how to proceed yet.

And 3D navigation is a good example. There are many elements that implement it in different ways, and I want them to share a single implementation.

About material types and exporters, they are defined in MM, not in the code (select Material node, Control+W, click the pencil button).

[bliepp]

I understand. But keep in mind that it might take a year or two until it makes sense to migrate to Godot 4.X for stability reasons. Like Godot 3, where there were API breaking changes between 3.0 and 3.1, this might be the case for Godot 4 as well. Until then I don't think it will be a waste of time as it is unclear when Godot 4.0 is released (were not even in beta right now) and how it's going to evolve.

Also, I would state that a refactored code is way easier to migrate and would actually prevent that a complete rewrite is necessary. GDScript for Godot 4 does not seem to break things fundamentally. Of course, some syntax changes and some API changes, but the overall coding structure seems to stay the same.

I agree that code refactoring needs some planning and structure and rushing wouldn't lead to neat results, this is why I opened this issue - to discuss what's best and what direction to choose.

[Zireael07]

I wouldn't say it'd take a year or two to move to Godot 4. I'm nearly certain we'll see beta this year, and likely 4.0 itself this year. But yes, I'd refactor before the move, it'd make the move easier.

[bliepp]

I wouldn't say it'd take a year or two to move to Godot 4. I'm nearly certain we'll see beta this year, and likely 4.0 itself this year. But yes, I'd refactor before the move, it'd make the move easier.

Actually I didn't mean that it would take 1-2 years to port it to Godot 4, I wanted to suggest to wait with moving to Godot 4 1-2 years as the past of Godot has shown that a brand new major release does not necessarily mean a freeze in terms of breaking changes. Godot 3.0 and 3.1 had significant incompatibilities so immediately moving to Godot 4 with the release of 4.0 would be a bad idea anyway as I would not consider it production ready when speaking of continuously developed software. For games with a final release date with only minor patches later Godot 4.0 may be suitable, but not for software with features added on a regular basis. So transitioning to Godot 4.X should be scheduled 1-2 years in the future anyway (as time will show how Godot 4 will evolve). Therefore I would not consider this an argument against refactoring now as RodZill4 argued. That was what I meant.

[Calinou]

Actually I didn't mean that it would take 1-2 years to port it to Godot 4, I wanted to suggest to wait with moving to Godot 4 1-2 years as the past of Godot has shown that a brand new major release does not necessarily mean a freeze in terms of breaking changes.

The versioning policy has changed since Godot 3.3, which means semantic versioning is now more closely followed.

[bliepp]

The versioning policy has changed since Godot 3.3, which means semantic versioning is now more closely followed.

So does this mean that a similar difference like between Godot 3.0 and 3.1 is likely not going to occur between Godot 4 minor releases?

[Calinou]

So does this mean that a similar difference like between Godot 3.0 and 3.1 is likely not going to occur between Godot 4 minor releases?

We'll try to avoid compatibility breakage as much as possible, unless there is no way around it to prevent a feature from being unusable throughout the 4.x cycle.

[bliepp]

We'll try to avoid compatibility breakage as much as possible, unless there is no way around it to prevent a feature from being unusable throughout the 4.x cycle.

Okay, nice to know, thanks.

Nevertheless my point about refactoring before migrating still holds.